Closed Bug 1096763 Opened 5 years ago Closed 5 years ago

Removing WebIDE toolbar button breaks about:customizing

Categories

(DevTools :: WebIDE, defect, major)

35 Branch
defect
Not set
major
Points:
3

Tracking

(firefox34 unaffected, firefox35 verified, firefox36 verified)

VERIFIED FIXED
Firefox 36
Iteration:
36.3
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)

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.
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
(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
Fun times.
Attachment #8520649 - Flags: review?(jryans)
Attachment #8520649 - Flags: review?(bmcbride)
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.
(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+
Duplicate of this bug: 1097650
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-
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)
Attachment #8520649 - Attachment is obsolete: true
Duplicate of this bug: 1094617
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-
(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.
Marco, can you add this?
Iteration: --- → 36.3
Points: --- → 3
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: in-testsuite+
Flags: firefox-backlog+
Added to IT 36.3
Flags: needinfo?(mmucci)
(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...
I *think* this is what you mean?
Attachment #8523062 - Flags: review?(bmcbride)
Attachment #8522205 - Attachment is obsolete: true
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dabf47d7f720
Status: ASSIGNED → RESOLVED
Closed: 5 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)
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?
Attachment #8523062 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: petruta.rasa
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.
Status: RESOLVED → VERIFIED
Iteration: 36.3 → 37.3
See Also: → 1393661
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.