Closed Bug 1452889 Opened 6 years ago Closed 6 years ago

Handle appending multiple elements to a listbox correctly.

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

See the panic in bug 1446368.
Comment on attachment 8966509 [details]
Bug 1452889: Handle appending multiple items to a listbox correctly.

https://reviewboard.mozilla.org/r/235230/#review243818

::: commit-message-1ccce:7
(Diff revision 1)
> +
> +What happened in bug 1446368 is the following: We append two items to an empty
> +listbox.
> +
> +We can't construct lazily because this is XUL, so that goes through
> +IssueSingleInsertNotifications for each of both. When we insert the first one we

"for each of the items"
Attachment #8966509 - Flags: review?(bzbarsky) → review+
Comment on attachment 8966510 [details]
Bug 1452889: Assert more tightly.

https://reviewboard.mozilla.org/r/235232/#review243822

::: layout/base/nsCSSFrameConstructor.cpp:5561
(Diff revision 1)
>    // XXX the GetContent() != aContent check is needed due to bug 135040.
>    // Remove it once that's fixed.
>    if (aContent->GetPrimaryFrame() &&
>        aContent->GetPrimaryFrame()->GetContent() == aContent &&
>        !aState.mCreatingExtraFrames) {
> +    MOZ_ASSERT(MaybeGetListBoxBodyFrame(aContent),

I don't follow this.  We already have an unconditional NS_ERROR here.  What is this looser assert adding?

Of is that NS_ERROR triggering at the moment?
Attachment #8966510 - Flags: review?(bzbarsky) → review-
Comment on attachment 8966510 [details]
Bug 1452889: Assert more tightly.

https://reviewboard.mozilla.org/r/235232/#review243822

> I don't follow this.  We already have an unconditional NS_ERROR here.  What is this looser assert adding?
> 
> Of is that NS_ERROR triggering at the moment?

That NS_ERROR triggers (with the listbox) in bug 1446368. I want to know if something that isn't a listbox hits this.
Comment on attachment 8966510 [details]
Bug 1452889: Assert more tightly.

https://reviewboard.mozilla.org/r/235232/#review243830

r=me if you add a comment explaining why both assertions are there.

::: commit-message-6b3f9:1
(Diff revision 1)
> +Bug 1452889: Assert more tightly. r?bz

This is actually a looser assert, just more fatal, right?
Attachment #8966510 - Flags: review- → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02c0f1ec90d0
Handle appending multiple items to a listbox correctly. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/38b392d4b4c1
Assert fatally if we're constructing frames for something that has frames and isn't a listbox item. r=bz
https://hg.mozilla.org/mozilla-central/rev/02c0f1ec90d0
https://hg.mozilla.org/mozilla-central/rev/38b392d4b4c1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.