Closed Bug 976792 Opened 6 years ago Closed 6 years ago

CustomizableUI's insertNodeInWindow should go through placements and try followup next nodes

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: Gijs, Assigned: Unfocused)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 file, 2 obsolete files)

Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Blair, how is this going and/or can I help? :-)
Flags: needinfo?(bmcbride)
Main part of the patch is done, just gotta put time aside to expand the test coverage - because in fixing this, I discovered a few extra bugs in the old code. Bit surprised none of us noticed this sooner :\
Flags: needinfo?(bmcbride)
Fixing old bugs and writing additional tests sounds like a task for a followup non-Australis:P2 bug?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #3)
> Fixing old bugs and writing additional tests sounds like a task for a
> followup non-Australis:P2 bug?

I suspect 'old' isn't as 'old' as you think. :-)
Yea, no, I meant "old" as in the existing new-ish code we rely on that I had to re-write that code to fix this bug anyway. I wanted tests to properly cover the new code, covering the classes of issues I found in the code I rewrote.

Though, turns out even that involved yak shaving with finding more bugs. Trying to work around that now, and put it into followup bugs. Though I think we'll want a P2 out of that :\
So, update: Still banging my head against the desk on this. Tests are... not great, but good enough (bug 980155 got in the way plenty). But I'm fighting issues around overflow toolbars, which the old code handled blatantly wrong. Now I've fixed that I have a test failing - but that's good, because it was accidentally passing before, and it's actually indirectly testing something bug 980155 was preventing me from testing (overflowable toolbars and non-overflowable widgets).
Attached patch WiP v1 (obsolete) — Splinter Review
Gijs, maybe you could share some brainpower. I'm stumped on finding an algorithm that works for finding the right place for a node in an overflowing toolbar, and widgets that disallow overflowing.

ie, placements looks something like:
  exists-1,exists-2,overflows-1,trying-to-insert-this,can't-overflow-1,overflows-2

So the actual DOM in the toolbar looks like:
  exists-1,exists-2,can't-overflow-1
And in the overflow list:
  overflows-1,overflows-2



And then there's the variation of having multiple widgets that don't allow overflowing, and and cases where we have widgets in various positions the placements but don't exist in the DOM.
Attachment #8387554 - Flags: feedback?(gijskruitbosch+bugs)
Blocks: 980155
I think this is correct. Blair, can you verify? All the tests you added so far are passing. I'll look at the other patch you uploaded next.
Attachment #8387640 - Flags: review?(bmcbride)
Attachment #8387554 - Attachment is obsolete: true
Attachment #8387554 - Flags: feedback?(gijskruitbosch+bugs)
Assignee: bmcbride → gijskruitbosch+bugs
Bad bzexport... :-(
Assignee: gijskruitbosch+bugs → bmcbride
Comment on attachment 8387640 [details] [diff] [review]
CustomizableUI's insertNodeInWindow should go through placements and try followup next nodes

Review of attachment 8387640 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on your modifications (thankyou!), they work beautifully.

New patch coming up with more thorough test coverage.
Attachment #8387640 - Flags: review?(bmcbride) → review+
Attached patch Patch v2Splinter Review
Attachment #8387640 - Attachment is obsolete: true
Attachment #8388043 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8388043 [details] [diff] [review]
Patch v2

Oh, I should note: This applies *on top of* bug 980155 (previously it was the other way around). Which let me expand the test suite such that I'm finally happy with the test coverage.
Comment on attachment 8388043 [details] [diff] [review]
Patch v2

Review of attachment 8388043 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but same comment as on the other test... it'd be nice if we reduced the number of widgets these tests used, and made their width more deterministic (so we can guarantee that it works on all platforms).

Also... IME these resizing+overflow tests are funky on Linux, and I've disabled all the other ones. For aurora uplift at least, I think we should do the same here, although I'm somewhat hopeful that my fixes to the tests that open new windows (to wait longer for the new windows to have completely closed) will have helped.

Generally, the code should work the same cross-platform and where it doesn't, that's likely as not a core problem, so I think the loss of test coverage on one platform wouldn't be terrible.

::: browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
@@ +53,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 0; i < 20; i++) {

Here, too, it would be swell if we could decrease the number of widgets we're using.

@@ +98,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 0; i < 20; i++) {

And here.

@@ +144,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 0; i < 20; i++) {

And here.

@@ +191,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 30; i >= 0; i--) {

And definitely here. :-)

@@ +199,5 @@
> +    CustomizableUI.createWidget(spec);
> +    CustomizableUI.addWidgetToArea(id, "nav-bar", 0);
> +  }
> +
> +  for (let i = 40; i < 50; i++) {

I think you get the idea now... :-)
Attachment #8388043 - Flags: review?(gijskruitbosch+bugs) → review+
I reduced the number of widgets in the test and pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/8f6765a92092
Flags: in-testsuite+
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8f6765a92092
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
Comment on attachment 8388043 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: insertion of nodes is unpredictable when using the provided APIs
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): low due to automated test
String or IDL/UUID changes made by this patch: none
Attachment #8388043 - Flags: approval-mozilla-aurora?
Attachment #8388043 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.