842065 - Remove nsIFrame::GetStyleDataExternal

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

nsIFrame::GetStyleDataExternal() can go away.

IIUC believe it's there as a virtual method to allow other libraries (i.e. non-gklayout libraries, in non-libxul builds) to call into methods like GetStyleVisiblity() that need access to our mStyleContext.

However, now that we no longer support non-libxul builds, it's no longer necessary.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 2

[Daniel Holbert [:dholbert]]

Actually, maybe _IMPL_NS_LAYOUT is defined unconditionally now, so we can just drop that #if/#else in nsIFrame.h?

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 714824 [details] [diff] [review]
fix v1

>diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h
> #ifdef _IMPL_NS_LAYOUT
>   #define STYLE_STRUCT(name_, checkdata_cb_, ctor_args_)                      \
>     const nsStyle##name_ * GetStyle##name_ () const {                         \
>       NS_ASSERTION(mStyleContext, "No style context found!");                 \
>       return mStyleContext->GetStyle##name_ ();                               \
>     }
> #else
>   #define STYLE_STRUCT(name_, checkdata_cb_, ctor_args_)                      \
>     const nsStyle##name_ * GetStyle##name_ () const {                         \
>+      NS_ASSERTION(mStyleContext, "unexpected null pointer");                 \
>       return static_cast<const nsStyle##name_*>(                              \
>-                            GetStyleDataExternal(eStyleStruct_##name_));      \
>+        mStyleContext->GetStyleData(eStyleStruct_##name_));                   \
>     }

Yeah, never mind -- looks like we should only be taking this patch IFF we can assume _NS_IMPL_LAYOUT is always defined (which IIUC means "we're in the same library as gklayout").

And if that assumption is true, then we can just drop the "#else" clause here entirely. Right?

(Would it be safe to stop defining / checking _NS_IMPL_LAYOUT?)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If there are no longer any callers outside libxul (possibly including binary extensions... though I hope not), then we could get rid of the whole not-_IMPL_NS_LAYOUT path.

(It also could be a perf win, since w don't actually bother to define _IMPL_NS_LAYOUT for many things outside layout/ that are in libxul; there might even be other uses of _IMPL_NS_LAYOUT that depend on that, though I suspect not.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

It would slightly surprise me if we do not have binary extensions getting style data...

Comment 6

[Daniel Holbert [:dholbert]]

Hmm... yeah, I didn't realize that binary extensions might be using this.

Given that possibility and given that this API isn't currently breaking / blocking anything, we might as well keep it for the time being.

Comment 7

[Benjamin Smedberg]

I'd be surprised if binary extensions were using nsIFrame nowadays... it's almost impossible to get to.

Previously we tried to add an #error to nsIFrame.h if you weren't MOZILLA_INTERNAL_API but there were issues with header include dependencies pulling in frames from content headers. Perhaps that could be resurrected, though.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So maybe we should do this; it should get us a perf win given the _IMPL_NS_LAYOUT not being defined for everything in libxul.

Comment 9

[Daniel Holbert [:dholbert]]

Note also that Bug 781360 patch 4 renamed GetStyleDataExternal to just StyleDataExternal (though maybe that doesn't affect binary extension compatibility?)

Circling back to this; I'm going to unbitrot & re-post the patch.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix v1a (unbitrotted)

Here's an unbitrotted version of fix v1 (mostly just dropping a lot of "Get" prefixes).

I'm not clear from comment 8 on whether we should also drop the non-_IMPL_NS_LAYOUT chunk or keep it; I'd suggested that because I mistakenly thought it was unconditionally defined, but if it's not, then maybe we still want the two separate behaviors?

Comment 11

[Daniel Holbert [:dholbert]]

See also bug 659722; apparently _IMPL_NS_LAYOUT may be replaced with MOZILLA_INTERNAL_API soon.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 740618 [details] [diff] [review]
fix v1a (unbitrotted)

>-  /**
>    * Define typesafe getter functions for each style struct by
>    * preprocessing the list of style structs.  These functions are the
>    * preferred way to get style data.  The macro creates functions like:
>    *   const nsStyleBorder* StyleBorder();
>    *   const nsStyleColor* StyleColor();
>    */
> 
> #ifdef _IMPL_NS_LAYOUT
>   #define STYLE_STRUCT(name_, checkdata_cb_)                                  \
>     const nsStyle##name_ * Style##name_ () const {                            \
>       NS_ASSERTION(mStyleContext, "No style context found!");                 \
>       return mStyleContext->Style##name_ ();                                  \
>     }
> #else
>   #define STYLE_STRUCT(name_, checkdata_cb_)                                  \
>     const nsStyle##name_ * Style##name_ () const {                            \
>+      NS_ASSERTION(mStyleContext, "unexpected null pointer");                 \
>       return static_cast<const nsStyle##name_*>(                              \
>-                            StyleDataExternal(eStyleStruct_##name_));         \
>+        mStyleContext->StyleData(eStyleStruct_##name_));                      \
>     }
> #endif

You should just remove the #ifdef-ing here as well (leaving only the first half)


You should also add a comment to both places that callers that are outside of libxul should be using nsIDOMWindow::GetComputedStyle instead.

r=dbaron with that

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> You should just remove the #ifdef-ing here as well (leaving only the first
> half)

Done. (& fixed minor bitrot)

> You should also add a comment to both places that callers that are outside
> of libxul should be using nsIDOMWindow::GetComputedStyle instead.

Sorry -- I'm not sure what you mean by "both places" there. I've added a comment in one place, at the end of the "Define typesafe getter functions" comment-block, in this updated patch -- where was the other place it should go?

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 752188 [details] [diff] [review]
fix v2

(In reply to Daniel Holbert [:dholbert] from comment #13)
> where was the other place it should go?

dbaron says there used to be a second place, but it looks like it's been removed.

So I think this is good to land! I'll give it a sanity-check set of try builds, and push it to inbound if nothing explodes.

Comment 15

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6f7fd5019c99

Comment 16

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1823d1612f0

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d1823d1612f0