Closed
Bug 1096763
Opened 10 years ago
Closed 10 years ago
Removing WebIDE toolbar button breaks about:customizing
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(firefox34 unaffected, firefox35 verified, firefox36 verified)
VERIFIED
FIXED
Firefox 36
Tracking | Status | |
---|---|---|
firefox34 | --- | unaffected |
firefox35 | --- | verified |
firefox36 | --- | verified |
People
(Reporter: sergemp, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
8.51 KB,
patch
|
Unfocused
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0 Build ID: 20141110004002 Steps to reproduce: 0. Download firefox aurora / developers edition. 1. Run `firefox -p`, create empty profile and start it. 2. Right-click on WebIDE toolbar button, select "Remove from Toolbar". 3. Quit Firefox, run it again with same profile. See the button still on a toolbar. 4. Again Right-click on WebIDE toolbar button and select remove. 5. Open New Window (Menu / New Window), select Menu / Customize. That's all, about:customizing is broken in any new window now. I've accidentally hit this after upgrading existing Firefox Aurora profile. It worked fine in earlier Firefox Aurora versions, like https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-05-00-40-06-mozilla-aurora/firefox-35.0a2.en-US.linux-i686.tar.bz2 Actual results: WebIDE button stays on a toolbar after Firefox restart. And when it's gone, about:customizing does not work in new windows any more. Expected results: WebIDE should leave toolbar on the first try. Customizing Firefox page should work in any window.
Assignee | ||
Comment 1•10 years ago
|
||
Ugh. The second part is because it calls createWidget for every window. I haven't figured out the first part yet, but it's probably related.
Blocks: 1056923
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: WebIDE
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
I'd like to help out here, but I am not sure I follow how CustomizableUI is meant to be used correctly. (In reply to :Gijs Kruitbosch from comment #1) > The second part is because it calls createWidget for every window. So we should instead call it once for the whole browser session? Are we also meant to try looking for the widget by ID first, before automatically calling createWidget? I see that is what's done in the social widget case[1], for example. [1]: http://dxr.mozilla.org/mozilla-central/source/browser/modules/Social.jsm#267
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #2) > I'd like to help out here, but I am not sure I follow how CustomizableUI is > meant to be used correctly. > > (In reply to :Gijs Kruitbosch from comment #1) > > The second part is because it calls createWidget for every window. > > So we should instead call it once for the whole browser session? Yes. > Are we > also meant to try looking for the widget by ID first, before automatically > calling createWidget? I see that is what's done in the social widget > case[1], for example. That's just it trying to do the above. If this was in a JSM that was only loaded once, and in a method that was only called once, you wouldn't have this problem. The webide button does actually do a similar thing already, but for some reason it's using widget.instances instead of the provider check as done above, which is wrong. There's also a very subtle issue with 3rd-party widgets at play here, which I'll also be fixing. However, I don't understand why this wasn't added as a builtin widget (in CustomizableWidgets.jsm) to begin with. I'm assuming there were reasons?
Summary: [regression] Removing WebIDE toolbar button breaks about:customizing → Removing WebIDE toolbar button breaks about:customizing
Assignee | ||
Comment 4•10 years ago
|
||
Fun times.
Attachment #8520649 -
Flags: review?(jryans)
Attachment #8520649 -
Flags: review?(bmcbride)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs Kruitbosch from comment #3) > There's also a very subtle issue with 3rd-party widgets at play here, which > I'll also be fixing. However, I don't understand why this wasn't added as a > builtin widget (in CustomizableWidgets.jsm) to begin with. I'm assuming > there were reasons? Perhaps this was done to "hide" the button until WebIDE is opened the first time? But I am just guessing, and perhaps it could still have this behavior while being built-in. Paul's implementation in bug 1056923 was heavily guided by :jaws.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #5) > (In reply to :Gijs Kruitbosch from comment #3) > > There's also a very subtle issue with 3rd-party widgets at play here, which > > I'll also be fixing. However, I don't understand why this wasn't added as a > > builtin widget (in CustomizableWidgets.jsm) to begin with. I'm assuming > > there were reasons? > > Perhaps this was done to "hide" the button until WebIDE is opened the first > time? But it isn't... it's present on the toolbar by default in new profiles, AFAICT?
(In reply to :Gijs Kruitbosch from comment #6) > (In reply to J. Ryan Stinnett [:jryans] from comment #5) > > (In reply to :Gijs Kruitbosch from comment #3) > > > There's also a very subtle issue with 3rd-party widgets at play here, which > > > I'll also be fixing. However, I don't understand why this wasn't added as a > > > builtin widget (in CustomizableWidgets.jsm) to begin with. I'm assuming > > > there were reasons? > > > > Perhaps this was done to "hide" the button until WebIDE is opened the first > > time? > > But it isn't... it's present on the toolbar by default in new profiles, > AFAICT? That's only the case in Dev Edition[1]. For other channels, it happens after first open[2] of WebIDE. [1]: http://hg.mozilla.org/releases/mozilla-aurora/file/c8866291dba2/browser/devtools/webide/webide-prefs.js#l26 [2]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/webide.js#99
Comment on attachment 8520649 [details] [diff] [review] fix webide button issues with customize mode, Review of attachment 8520649 [details] [diff] [review]: ----------------------------------------------------------------- I have only reviewed browser/devtools/*. I tested this locally, and it appears to preserve correct functionality for both Dev Edition and other channels.
Attachment #8520649 -
Flags: review?(jryans) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8520649 [details] [diff] [review] fix webide button issues with customize mode, Review of attachment 8520649 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/CustomizableUI.jsm @@ +2062,5 @@ > + // removed and then readded after a restart, because we "forgot" it > + // was there before. > + if (widget.source == CustomizableUI.SOURCE_EXTERNAL && > + !gSeenWidgets.has(widget.id)) { > + gSeenWidgets.add(widget.id); So, I think there are more potential bugs here. I can't remember all the details of the history around this code, but I'm pretty sure a widget should always be added to gSeenWidgets; but there are cases when it isn't. So I think you should refactor this so it's always added to gSeenWidgets, after this if/else block (note the try/finally block).
Attachment #8520649 -
Flags: review?(bmcbride) → review-
Assignee | ||
Comment 11•10 years ago
|
||
I think this should happen before firing onWidgetAdded and/or doing the auto-add, though, so that we save state in the right circumstances and don't change existing behaviour (as other code in the callgraph from those two actions might expect gSeenWidgets to have been updated). Other than that, is this what you mean?
Attachment #8522205 -
Flags: review?(bmcbride)
Assignee | ||
Updated•10 years ago
|
Attachment #8520649 -
Attachment is obsolete: true
Comment 13•10 years ago
|
||
Comment on attachment 8522205 [details] [diff] [review] fix webide button issues with customize mode, Review of attachment 8522205 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/CustomizableUI.jsm @@ +2075,5 @@ > // If the widget doesn't have an existing placement, and it hasn't been > // seen before, then add it to its default area so it can be used. > // If the widget is not removable, we *have* to add it to its default > // area here. > let canBeAutoAdded = autoAdd && !gSeenWidgets.has(widget.id); It needs to be added after this check here. Little concerning if that didn't result in a test failure.
Attachment #8522205 -
Flags: review?(bmcbride) → review-
Comment 14•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #11) > (as other code in the callgraph from those two > actions might expect gSeenWidgets to have been updated) createWidget should be the only thing that really uses gSeenWidgets. Everything else is just background management around clearing/restoring.
Blocks: 1098589
status-firefox34:
--- → unaffected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
Assignee | ||
Comment 15•10 years ago
|
||
Marco, can you add this?
Iteration: --- → 36.3
Points: --- → 3
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: in-testsuite+
Flags: firefox-backlog+
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #13) > Comment on attachment 8522205 [details] [diff] [review] > fix webide button issues with customize mode, > > Review of attachment 8522205 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/customizableui/CustomizableUI.jsm > @@ +2075,5 @@ > > // If the widget doesn't have an existing placement, and it hasn't been > > // seen before, then add it to its default area so it can be used. > > // If the widget is not removable, we *have* to add it to its default > > // area here. > > let canBeAutoAdded = autoAdd && !gSeenWidgets.has(widget.id); > > It needs to be added after this check here. Little concerning if that didn't > result in a test failure. Nah, it's a little concerning that I don't run unit tests after "minor" adjustments to a patch... (there are several failing tests with the attached patch) (In reply to Blair McBride [:Unfocused] from comment #14) > (In reply to :Gijs Kruitbosch from comment #11) > > (as other code in the callgraph from those two > > actions might expect gSeenWidgets to have been updated) > > createWidget should be the only thing that really uses gSeenWidgets. > Everything else is just background management around clearing/restoring. Mm, I should have said something else: setting gDirty after the call to endBatchUpdate is not very useful, and doesn't force a pref flush here. Simply moving the block does seem to fix the testcase as written though, so I need to look a bit more into how well it does/doesn't reflect what happens if I actually restart, or if there's some other reason we will do a second pref save if I add the widget to gSeenWidgets after endBatchUpdate...
Assignee | ||
Comment 18•10 years ago
|
||
I *think* this is what you mean?
Attachment #8523062 -
Flags: review?(bmcbride)
Assignee | ||
Updated•10 years ago
|
Attachment #8522205 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
Comment on attachment 8523062 [details] [diff] [review] fix webide button issues with customize mode, Review of attachment 8523062 [details] [diff] [review]: ----------------------------------------------------------------- Yep. Sorry, was assuming a bunch of things when explaining this earlier - such as the need to move the batch start/end operations.
Attachment #8523062 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 20•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=35bf1329b0b6
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/dabf47d7f720
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dabf47d7f720
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Gijs, I believe we should uplift this to Dev Edition.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8523062 [details] [diff] [review] fix webide button issues with customize mode, Yes. Approval Request Comment [Feature/regressing bug #]: dev edition webide button [User impact if declined]: large-scale breakage if you try to get rid of the button on dev edition [Describe test coverage new/current, TBPL]: has some automated testing for the customizableui fixes in here, otherwise needs manual testing [Risks and why]: slim to none, the difficult changes here have tests, the single-line change in devtools is straightforward enough that I'm not worried [String/UUID change made/needed]: no
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8523062 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8523062 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
QA Contact: petruta.rasa
Comment 25•9 years ago
|
||
Reproduced using Dev Edition 2014-11-10. On latest Nightly 36.0a1 2014-11-22, under Win 7 64-bit, Ubuntu 14.04 32-bit and Mac OSX 10.9.5, the WebIDE icon from toolbar is correctly removed. Will verify DevEd 35.0a2 once the fix lands there.
Updated•9 years ago
|
Iteration: 37.3 → 36.3
Comment 27•9 years ago
|
||
Verified as fixed using Firefox 35 beta 1.
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•