-
Notifications
You must be signed in to change notification settings - Fork 1
/
review_advice
66 lines (44 loc) · 3.04 KB
/
review_advice
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
Your nominated add-on, nextpage, has been reviewed by a Mozilla Add-ons editor who decided to retain your add-on in the sandbox.
Review Information:
Reviewer: Yair Halevi
Comments: The reasons I'm retaining this in the sandbox:
* DONE The following errors were found in the Error Console while testing your add-on:
Error: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMWindowInternal.back]
Source file: chrome://browser/content/browser.xul
Line: 1
While this error doesn't seem to come from your code, it is a result of the
addon being installed. I believe this happens when you click p or 1 and there
is no history.
** FIXED [review 1] error message when click p or 1 and there is no history
see commit 88d94c2
* TODO The text "try to go to the next page" that appears next to the menu option (in the View menu) is located at a position where the firefox UI standard dictates the shortcut key should be specified (see the other menu options). You shouldn't use that space for anything other than specifying the shortcut key (especially since it isn't the purpose of your addon to modify the standard look and feel of firefox).
** I can't see this problem on my firefox
According to https://developer.mozilla.org/en/XUL/menuitem, the acceltext
attribute is used to show shortcut key, I'm using the description attribute
for "try to go to the next page".
Related source code:
<menuitem id="nextpage-nextpage"
label="&nextpage.label;"
accesskey="&nextpage.accesskey;"
description="&nextpage.description;"
oncommand="nextpage.goto_next_page();"/>
Some additional comments, that won't prevent me from approving the add-on, but I think you should consider:
* The add-on worked for me on google search results and bing search results, but I also tried it in yahoo (didn't work) and in amazon search results, in which case it just kept bringing me back to the top of the the same page.
Yahoo search doesn't work, it jumps to top of page. Added to TODO file, I'll
fix it later.
amazon.com search works for me.
* Since you are overriding quite a number of keys that many web pages may use for other purposes, it would be very useful to allow the user to change the keys used via a preferences dialog.
year, I know. That's on my TODO list, but with a low priority.
* TODO The files you uploaded for linux and windows are identical - please change the submission so that a single file is uploaded for both operating systems (so the editor knows there is no need to review both).
Could you tell me how to do that? When I upload a new version, and check
support linux and windows checkbox, two files are generated automatically for me.
* I suggest you specify a bit more clearly that the extension works in pages in English only (or the languages that are supported).
It works for pages in English and Chinese. Addon description updated.
A way to add language support without modifying the source code is on my TODO
list, with low priority.
Thanks for your review.
Rainy
Local Variables:
mode: org
mode: visual-line
End: