Closed
Bug 1026614
Opened 10 years ago
Closed 8 years ago
SDK ui/toolbar not working in permanent private browsing
Categories
(Add-on SDK Graveyard :: General, defect, P2)
Add-on SDK Graveyard
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: Kinneer_Erin, Assigned: zer0)
Details
Attachments
(2 files, 1 obsolete file)
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release) Build ID: 20140605174243 Steps to reproduce: Using the toolbar ui component example in the add-on sdk, I gave the toolbar extension permissions to be used in private browsing. In "permanent private browsing", the extension disappears. I need it to show in both "permanent private browsing" and window private browsing. Actual results: The extension disappeared. Expected results: The toolbar extension should have been in the browser.
Updated•10 years ago
|
Component: Untriaged → General
OS: Windows 7 → All
Product: Firefox → Add-on SDK
Hardware: x86_64 → All
Version: 30 Branch → unspecified
Updated•10 years ago
|
Flags: needinfo?(zer0)
Comment 1•10 years ago
|
||
Can confirm this is a bug, and also give you the places of where things go wrong. https://github.com/mozilla/addon-sdk/blob/f6bc93143d72c8a498099b39d8ddaf98a2071f52/lib/sdk/input/browser.js#L20 https://github.com/mozilla/addon-sdk/blob/f6bc93143d72c8a498099b39d8ddaf98a2071f52/lib/sdk/window/utils.js#L296 In input/browser.js, |windows| is called with |includePrivates: true|, while the actual option name is |includePrivate|. Hence private windows not enumerated to begin with, so no toolbars are ever created for private windows. STR: * Build a new add-on using the attached main.js and a package.json containing |"permissions": {"private-browsing": true}| (See: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/private-browsing) * cfx run * Open the browsr Preferences/Privacy and switch to Firefox will: Never remember history (aka. permanent private browsing) * Browser prompts to restart: Confirm -> After the browser restarts, the toolbar is missing in the initial window(s). -> Opening new windows will display the toolbar in these windows Expected: -> After the browser restarts, the toolbar is present and functional. But this is not all: After fixing the above, the toolbar will *always* show up, as the toolbar code does not consider the private-windows permission at all, e.g. by using |ignoreWindow|. ActionButtons do evaluate the private-browsing permission, hence the Toolbar will be empty in private windows. STR: * Fix the first bug, by correcting the option name in input/browser.js :p * In package.json, remove |"permissions": {"private-browsing": true}| again * cfx run (make sure the patched SDK is used) * Open a new private window -> An empty toolbar is displayed in the private window Expected: -> No toolbar should be displayed in the private window
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•10 years ago
|
||
Here is a quick and dirty patch. Please note however, that I'm not interested in taking this bug, going threw reviews, writing test, etc. The patch is purely for the information of whoever will fix this bug eventually.
Priority: -- → P2
Comment on attachment 8445722 [details] [diff] [review] Quick and dirty patch against the current master Matteo, care to review this?
Attachment #8445722 -
Flags: review?(zer0)
Attachment #8445722 -
Flags: review?(zer0)
This bug appears to also cause this code to open a new window instead of using the currently open "permanent private browsing" window. tabs.open({ url: url_to_open, isPrivate: true, inNewWindow: false });
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40889/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40889/
Attachment #8731869 -
Flags: review?(lgreco)
Assignee | ||
Comment 6•8 years ago
|
||
Luca, could you have a look? It's a quick review, we should probably check more edge cases, but at least it's better than now.
Flags: needinfo?(zer0)
Updated•8 years ago
|
Attachment #8731869 -
Flags: review?(lgreco) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8731869 [details] MozReview Request: Bug 1026614 - SDK ui/toolbar not working in permanent private browsing; r=rpl https://reviewboard.mozilla.org/r/40889/#review37571
Updated•8 years ago
|
Attachment #8445722 -
Attachment is obsolete: true
Comment 8•8 years ago
|
||
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #6) > Luca, could you have a look? It's a quick review, we should probably check > more edge cases, but at least it's better than now. The change and the new test cases looks good. I'd like to have another test case that actually tests that the toolbar is shown when the addon has the private-browsing permission (which would ensure that the issue described in the summary is fixed), but it seems that the test addon that should test this scenarios as been disabled: https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/addon-sdk/source/test/addons/jetpack-addon.ini#34 I tried to fix its most evident issue: const BUILTIN_SIDEBAR_MENUITEMS = exports.BUILTIN_SIDEBAR_MENUITEMS = [ 'menu_socialSidebar', 'menu_historySidebar', 'menu_bookmarksSidebar', + 'menu_tabsSidebar', ]; and to add the new test case for the ui toolbar, and it seems possible to fix and renable it. are there other reasons why the private-browsing-supported test addon is currently skipped on try?
Flags: needinfo?(zer0)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Luca Greco [:rpl] from comment #8) > are there other reasons why the private-browsing-supported test addon is > currently skipped on try? I don't know, I don't have much experience on add-on sdk tests on try since we moved from github. Probably Mossop knows better, since it seems the one who landed those changes. In the worst scenario, if no one knows, we can always try to re-enable and see what's happening. But if it's becoming too complicated, I would suggest to file a new bug for that and land in the meantime this one. Mossop, do you recall anything about it?
Flags: needinfo?(zer0) → needinfo?(dtownsend)
Comment 11•8 years ago
|
||
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #9) > But if it's becoming too complicated, I would suggest > to file a new bug for that and land in the meantime this one. sure, the test that you added ensures that the current behavior is preserved when it should be (when the addon doesn't have a private-browsing permission), and I'm ok with handling the opposite scenario (when the addon has the private-browsing permission and the toolbar should be attached) in a separate bugzilla issue.
Assignee: nobody → zer0
Keywords: checkin-needed
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/577b8fb1479a
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/577b8fb1479a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•