Closed
Bug 1389029
Opened 7 years ago
Closed 7 years ago
Make nsTableFrame stop calling ResolveInheritingAnonymousBoxStyle from attribute notifications.
Categories
(Core :: Layout: Tables, enhancement, P3)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: emilio, Assigned: neerja)
References
Details
Attachments
(2 files, 1 obsolete file)
That would've made the assertion in bug 1388234 hold, and it'd be nice in general.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → npancholi
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
When I was trying to verify the fix, I noticed that the original assert does not fail anymore (with or without my patch) on the latest build. But I'm still putting up this patch because it seems to be the right way - Try build with original assert added *without* my patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be65646bb6760766a7968ef49e4b3df30770b35&selectedJob=127562737 Try build with original assert added *with* my patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55dc97ee6f687dd126b2142b42de75782b76a86f&selectedJob=127561659
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8903446 [details] Bug 1389029 - Create custom change hint and post restyle event for rowspan, colspan attribute changes. https://reviewboard.mozilla.org/r/175208/#review180684 r=dbaron with the following changes ::: layout/base/nsCSSFrameConstructor.h:1816 (Diff revision 2) > RemoveFlags aFlags); > > + /** > + * Handles change of rowspan and colspan attributes on table cells. > + */ > + void UpdateTableCells(nsIContent* aContent); I'd update the name of this method to match the name of the hint (see below). ::: layout/base/nsCSSFrameConstructor.cpp:10001 (Diff revision 2) > + nsTableCellFrame* cellFrame = do_QueryFrame(aContent->GetPrimaryFrame()); > + if (cellFrame) { You should probably have a: NS_WARNING_ASSERTION(cellFrame, "hint should only be posted on table cells"); between these two lines, with the comment that it's possible that the warning could fire if some other style change simultaneously changes the 'display' of the element and makes it no longer be a table cell. ::: layout/base/nsChangeHint.h:245 (Diff revision 2) > > + /** > + * Indicates that there has been a colspan or rowspan attribute change > + * on the cells of a table. > + */ > + nsChangeHint_UpdateTableCells = 1 << 30, I'd probably call the hint UpdateTableCellSpans rather than UpdateTableCells. It's a little clearer about what it does. ::: layout/tables/nsTableFrame.cpp:358 (Diff revision 2) > -nsTableFrame::AttributeChangedFor(nsIFrame* aFrame, > - nsIContent* aContent, > +nsTableFrame::RowOrColSpanChanged(nsIFrame* aFrame, > + nsIContent* aContent) > - nsIAtom* aAttribute) > { > nsTableCellFrame *cellFrame = do_QueryFrame(aFrame); > if (cellFrame) { Given that this function's caller has an nsTableCellFrame*, you should change this function to take nsTableCellFrame* rather than nsIFrame*, and remove the do_QueryFrame. You should also remove the nsIContent* parameter.
Attachment #8903446 -
Flags: review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8903447 -
Attachment is obsolete: true
Pushed by npancholi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/65d4f7ef3fe6 Create custom change hint and post restyle event for rowspan, colspan attribute changes. r=dbaron
Assignee | ||
Updated•7 years ago
|
Attachment #8903447 -
Attachment is obsolete: false
Updated•7 years ago
|
Attachment #8903447 -
Flags: review?(emilio)
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8903447 [details] Bug 1389029 - Undo code changes in Bug 1388234 which is unnecessary now that we post restyle events for rowspan/colspan changes. https://reviewboard.mozilla.org/r/175210/#review181688 r=me, yay!
Attachment #8903447 -
Flags: review?(emilio)
Reporter | ||
Comment 10•7 years ago
|
||
Comment on attachment 8903447 [details] Bug 1389029 - Undo code changes in Bug 1388234 which is unnecessary now that we post restyle events for rowspan/colspan changes. (Mozreview didn't let me flag it because it said for some reason the change had been discarded)
Attachment #8903447 -
Flags: review+
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/65d4f7ef3fe6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 12•7 years ago
|
||
Re-opening for attachment 8903447 [details]
Status: RESOLVED → REOPENED
Flags: needinfo?(npancholi)
Resolution: FIXED → ---
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8903447 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8905228 -
Flags: review?(emilio)
Assignee | ||
Updated•7 years ago
|
Attachment #8903447 -
Attachment is obsolete: false
Assignee | ||
Updated•7 years ago
|
Attachment #8905228 -
Flags: review+
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > Comment on attachment 8903447 [details] > Bug 1389029 - Undo code changes in Bug 1388234 which is unnecessary now that > we post restyle events for rowspan/colspan changes. > > (Mozreview didn't let me flag it because it said for some reason the change > had been discarded) Sorry Emilio. Only way to get around this was to re-post this patch. Since you already r+ed the old one, I'm r+ing the same patch myself and unhidding the old one so that your r+ can be seen. Thanks!
Flags: needinfo?(npancholi)
Assignee | ||
Updated•7 years ago
|
Attachment #8903447 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8905228 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8905228 -
Flags: review?(emilio)
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Neerja Pancholi[:neerja] from comment #14) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > > Comment on attachment 8903447 [details] > > Bug 1389029 - Undo code changes in Bug 1388234 which is unnecessary now that > > we post restyle events for rowspan/colspan changes. > > > > (Mozreview didn't let me flag it because it said for some reason the change > > had been discarded) > > Sorry Emilio. Only way to get around this was to re-post this patch. Since > you already r+ed the old one, I'm r+ing the same patch myself and unhidding > the old one so that your r+ can be seen. > Thanks! Never mind! Mozreview is not letting me land this. Can you please give me an r+ again? Thanks!
Reporter | ||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8905228 [details] Bug 1389029 - Undo code changes in Bug 1388234 which is unnecessary now that we post restyle events for rowspan/colspan changes. https://reviewboard.mozilla.org/r/177016/#review182064
Attachment #8905228 -
Flags: review?(emilio) → review+
Comment 17•7 years ago
|
||
Pushed by npancholi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d29cfdf5b18f Undo code changes in Bug 1388234 which is unnecessary now that we post restyle events for rowspan/colspan changes. r=emilio
Updated•7 years ago
|
Priority: -- → P3
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d29cfdf5b18f
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•