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)

enhancement

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: nobody → npancholi
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 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+
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
Attachment #8903447 - Attachment is obsolete: false
Attachment #8903447 - Flags: review?(emilio)
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)
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+
https://hg.mozilla.org/mozilla-central/rev/65d4f7ef3fe6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Re-opening for attachment 8903447 [details]
Status: RESOLVED → REOPENED
Flags: needinfo?(npancholi)
Resolution: FIXED → ---
Attachment #8903447 - Attachment is obsolete: true
Attachment #8905228 - Flags: review?(emilio)
Attachment #8903447 - Attachment is obsolete: false
Attachment #8905228 - Flags: review+
(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)
Attachment #8903447 - Attachment is obsolete: true
Attachment #8905228 - Flags: review+
Attachment #8905228 - Flags: review?(emilio)
(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!
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+
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
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/d29cfdf5b18f
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.