Remove a bunch of IsCallerChrome callers

RESOLVED FIXED in Firefox 54

Status

()

Core
General
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(22 attachments, 4 obsolete attachments)

1.12 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.31 KB, patch
bholley
: review+
Details | Diff | Splinter Review
10.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.44 KB, patch
bholley
: review+
Details | Diff | Splinter Review
8.65 KB, patch
bholley
: review+
Details | Diff | Splinter Review
8.25 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.67 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
19.08 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
14.61 KB, patch
baku
: review+
Details | Diff | Splinter Review
10.95 KB, patch
smaug
: review-
Details | Diff | Splinter Review
11.56 KB, patch
jib
: review+
Details | Diff | Splinter Review
13.36 KB, patch
jib
: review+
Details | Diff | Splinter Review
2.12 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.69 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.63 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.10 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.91 KB, patch
bholley
: review+
Details | Diff | Splinter Review
15.49 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.84 KB, patch
bholley
: review+
Details | Diff | Splinter Review
9.27 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.02 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Just getting rid of this action-at-a-distance stuff, at least partly.
Created attachment 8832003 [details] [diff] [review]
part 1.  Stop using IsCallerChrome in DOMParser::Constructor
Attachment #8832003 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8832004 [details] [diff] [review]
part 2.  Stop using IsCallerChrome in CheckerboardReportService::IsEnabled
Attachment #8832004 - Flags: review?(bobbyholley)
Created attachment 8832005 [details] [diff] [review]
part 3.  Stop using IsCallerChrome in nsContentUtils::IsCutCopyAllowed()
Attachment #8832005 - Flags: review?(bugs)
Created attachment 8832006 [details] [diff] [review]
part 4.  Stop using IsCallerChrome in nsContentUtils::IsRequestFullScreenAllowed
Attachment #8832006 - Flags: review?(bobbyholley)
Created attachment 8832007 [details] [diff] [review]
part 5.  Stop using IsCallerChrome in nsINode
Attachment #8832007 - Flags: review?(bobbyholley)
Created attachment 8832008 [details] [diff] [review]
part 6.  Add a SystemCallerGuarantee enum that we use for [ChromeOnly, NeedsCallerType] cases
Attachment #8832008 - Flags: review?(bobbyholley)
Created attachment 8832009 [details] [diff] [review]
Byg  part 7.  Mostly stop using IsCallerChrome in nsObjectLoadingContent
Attachment #8832009 - Flags: review?(bobbyholley)
Created attachment 8832010 [details] [diff] [review]
part 8.  Stop using IsCallerChrome in UnwrapArgImpl
Attachment #8832010 - Flags: review?(bobbyholley)
Created attachment 8832011 [details] [diff] [review]
part 9.  Stop using IsCallerChrome in WebGL
Attachment #8832011 - Flags: review?(bobbyholley)
Created attachment 8832012 [details] [diff] [review]
part 10.  Stop using IsCallerChrome in CanvasRenderingContext2D
Attachment #8832012 - Flags: review?(bobbyholley)
Created attachment 8832013 [details] [diff] [review]
part 11.  Remove the use of ThreadsafeIsCallerChrome in Event::Init, since that's not really interested in the _caller_ anyway
Attachment #8832013 - Flags: review?(bugs)
Created attachment 8832015 [details] [diff] [review]
part 12.  Remove the use of IsCallerChrome in FetchRequest
Attachment #8832015 - Flags: review?(bkelly)
Created attachment 8832016 [details] [diff] [review]
part 13.  Remove the use of IsCallerChrome in dom/file
Attachment #8832016 - Flags: review?(amarchesini)
Created attachment 8832017 [details] [diff] [review]
part 14.  Remove the use of IsCallerChrome in IDBFactory
Attachment #8832017 - Flags: review?(bugs)
Created attachment 8832018 [details] [diff] [review]
part 15.  Remove the use of IsCallerChrome in ApplyConstraints
Attachment #8832018 - Flags: review?(jib)
Created attachment 8832019 [details] [diff] [review]
part 16.  Remove the use of IsCallerChrome in GetUserMedia
Attachment #8832019 - Flags: review?(jib)
Created attachment 8832020 [details] [diff] [review]
part 17.  Get rid of IsCallerChrome in the quota manager service

Maybe I should add an xpidl annotation for passing SystemCallerGuarantee...
Attachment #8832020 - Flags: review?(bobbyholley)
Created attachment 8832021 [details] [diff] [review]
part 18.  Get rid of IsCallerChrome in workers
Attachment #8832021 - Flags: review?(bobbyholley)
Created attachment 8832022 [details] [diff] [review]
part 19.  Get rid of IsCallerChrome in XMLDocument::Load
Attachment #8832022 - Flags: review?(bobbyholley)
Created attachment 8832023 [details] [diff] [review]
part 20.  Get rid of IsCallerChrome in XSLT
Attachment #8832023 - Flags: review?(peterv)
Created attachment 8832024 [details] [diff] [review]
part 21.  Get rid of some IsCallerChrome usage in XPConnect
Attachment #8832024 - Flags: review?(bobbyholley)
Created attachment 8832026 [details] [diff] [review]
part 22.  Get rid of IsCallerChrome in geometry utils
Attachment #8832026 - Flags: review?(dholbert)
Comment on attachment 8832016 [details] [diff] [review]
part 13.  Remove the use of IsCallerChrome in dom/file

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

::: dom/file/File.cpp
@@ +613,4 @@
>    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aGlobal.GetAsSupports());
>  
>    RefPtr<MultipartBlobImpl> impl = new MultipartBlobImpl(EmptyString());
> +  impl->InitializeChromeFile(window, aData, aBag, aGuarantee, aRv);

Don't pass aGuarantee here. InitializeChromeFile is chrome only as any BlobImpl.

::: dom/file/File.h
@@ +267,5 @@
>    virtual int64_t GetLastModified(ErrorResult& aRv) = 0;
>  
>    virtual void SetLastModified(int64_t aLastModified) = 0;
>  
> +  virtual void GetMozFullPath(nsAString& aName, SystemCallerGuarantee,

aGuarantee

@@ +413,5 @@
>  
>    virtual void SetLastModified(int64_t aLastModified) override;
>  
> +  virtual void GetMozFullPath(nsAString& aName,
> +                              SystemCallerGuarantee,

aGuarantee

::: dom/file/MultipartBlobImpl.cpp
@@ +327,5 @@
>  MultipartBlobImpl::InitializeChromeFile(nsPIDOMWindowInner* aWindow,
>                                          nsIFile* aFile,
>                                          const ChromeFilePropertyBag& aBag,
>                                          bool aIsFromNsIFile,
> +                                        SystemCallerGuarantee,

Don't pass this argument.

@@ -335,5 @@
>      aRv.Throw(NS_ERROR_UNEXPECTED);
>      return;
>    }
>  
> -  MOZ_ASSERT(nsContentUtils::IsCallerChrome());

Can we keep this assertion and get rid of the SystemCallerGuarantee argument?

@@ +397,5 @@
>  void
>  MultipartBlobImpl::InitializeChromeFile(nsPIDOMWindowInner* aWindow,
>                                          const nsAString& aData,
>                                          const ChromeFilePropertyBag& aBag,
> +                                        SystemCallerGuarantee aGuarantee,

same here.

::: dom/file/MultipartBlobImpl.h
@@ +70,5 @@
>    void InitializeChromeFile(nsPIDOMWindowInner* aWindow,
>                              nsIFile* aData,
>                              const ChromeFilePropertyBag& aBag,
>                              bool aIsFromNsIFile,
> +                            SystemCallerGuarantee,

aGuarantee
Attachment #8832016 - Flags: review?(amarchesini) → review+
> Don't pass aGuarantee here. InitializeChromeFile is chrome only as any BlobImpl.

I'm not sure I follow.  The whole point is that having this argument guarantees we have a system caller, hence we can remove the IsCallerChrome() assert, since we have an argument that says caller is system.

> aGuarantee

Why, if it's not used?  I sort of like the pattern of just the bare "SystemCallerGuarantee" to mean "called as system".  I have it as aGuarantee in some places that need to propagate that state to someone else just so I can avoid typing out SystemCallerGuarantee::ThisCallerIsSystem...

I can add the unused aGuarantee if you really want, of course.
Flags: needinfo?(amarchesini)
> I can add the unused aGuarantee if you really want, of course.

I don't know if we have a guideline for this thing. unused seems better to me.
Flags: needinfo?(amarchesini)
Comment on attachment 8832005 [details] [diff] [review]
part 3.  Stop using IsCallerChrome in nsContentUtils::IsCutCopyAllowed()

The change to nsHTMLDocument::EditingStateChanged() is a bit ugly. Passing somewhat random principal. But fine.
Attachment #8832005 - Flags: review?(bugs) → review+

Updated

a year ago
Attachment #8832015 - Flags: review?(bkelly) → review+
Comment on attachment 8832017 [details] [diff] [review]
part 14.  Remove the use of IsCallerChrome in IDBFactory

The mainthread checks shouldn't be removed.

It might be nice to have some way to pass SystemCallerGuarantee to other methods taking CallerType, but not about this patch.
Attachment #8832017 - Flags: review?(bugs) → review-

Updated

a year ago
Attachment #8832013 - Flags: review?(bugs) → review+
Comment on attachment 8832026 [details] [diff] [review]
part 22.  Get rid of IsCallerChrome in geometry utils

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

r=me, though some of this probably wants to be in an earlier patch, as noted below:

::: dom/base/nsINode.h
@@ +285,5 @@
>    typedef mozilla::dom::DOMQuad DOMQuad;
>    typedef mozilla::dom::DOMRectReadOnly DOMRectReadOnly;
>    typedef mozilla::dom::OwningNodeOrString OwningNodeOrString;
>    typedef mozilla::dom::TextOrElementOrDocument TextOrElementOrDocument;
> +  typedef mozilla::dom::CallerType CallerType;

If you like, it'd probably be cleaner to fold this change (& the chunk directly below it) into part 5 -- the part with other nsINode changes.

@@ +1776,5 @@
>    // when accessed from chrome privileged script and
>    // from content privileged script for compatibility.
>    void GetBaseURIFromJS(nsAString& aBaseURI,
> +                        CallerType aCallerType,
> +                        ErrorResult& aRv) const;

(This is the other chunk to fold into part 5, if you like -- this is just dropping some namespaces in lines that you created/modified in part 5. This folding would reduce needless code-churn & make this patch a bit more targeted at actual GeometryUtils changes. Not a big deal, though.)
Attachment #8832026 - Flags: review?(dholbert) → review+
Created attachment 8832171 [details] [diff] [review]
part 5.  Stop using IsCallerChrome in nsINode
Attachment #8832171 - Flags: review?(bobbyholley)
Attachment #8832007 - Attachment is obsolete: true
Attachment #8832007 - Flags: review?(bobbyholley)
Created attachment 8832172 [details] [diff] [review]
part 6.  Add a SystemCallerGuarantee enum that we use for [ChromeOnly, NeedsCallerType] cases
Attachment #8832172 - Flags: review?(bobbyholley)
Attachment #8832008 - Attachment is obsolete: true
Attachment #8832008 - Flags: review?(bobbyholley)
Created attachment 8832173 [details] [diff] [review]
Byg  part 7.  Mostly stop using IsCallerChrome in nsObjectLoadingContent
Attachment #8832173 - Flags: review?(bobbyholley)
Attachment #8832009 - Attachment is obsolete: true
Attachment #8832009 - Flags: review?(bobbyholley)
Created attachment 8832174 [details] [diff] [review]
part 6.  Add a SystemCallerGuarantee class that we use for [ChromeOnly, NeedsCallerType] cases
Attachment #8832174 - Flags: review?(bobbyholley)
Attachment #8832172 - Attachment is obsolete: true
Attachment #8832172 - Flags: review?(bobbyholley)
> aGuarantee

Per IRC discussion, will do /* unused */.

> The mainthread checks shouldn't be removed.

Will fix.

> It might be nice to have some way to pass SystemCallerGuarantee to other methods taking CallerType

Done, by making SystemCallerGuarantee a class with "operator CallerType()" on it.

> If you like, it'd probably be cleaner to fold this change (& the chunk directly below it) into part 5

Good catch, done.
Attachment #8832003 - Flags: review?(bobbyholley) → review+
Attachment #8832004 - Flags: review?(bobbyholley) → review+
Attachment #8832006 - Flags: review?(bobbyholley) → review+
Attachment #8832011 - Flags: review?(bobbyholley) → review+
Attachment #8832012 - Flags: review?(bobbyholley) → review+
Attachment #8832020 - Flags: review?(bobbyholley) → review+
Attachment #8832021 - Flags: review?(bobbyholley) → review+
Attachment #8832022 - Flags: review?(bobbyholley) → review+
Attachment #8832024 - Flags: review?(bobbyholley) → review+
Attachment #8832171 - Flags: review?(bobbyholley) → review+
Attachment #8832173 - Flags: review?(bobbyholley) → review+
Comment on attachment 8832174 [details] [diff] [review]
part 6.  Add a SystemCallerGuarantee class that we use for [ChromeOnly, NeedsCallerType] cases

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

::: dom/base/nsGlobalWindow.h
@@ +1213,5 @@
>      if (mDoc) {
>        mDoc->WarnOnceAbout(nsIDocument::eWindow_Content);
>      }
> +    // We can use CallerType::System, because we have a SystemCallerGuarantee.
> +    GetContent(aCx, aRetval, mozilla::dom::CallerType::System, aError);

Can't you just pass aCallerType?
Attachment #8832174 - Flags: review?(bobbyholley) → review+
Attachment #8832010 - Flags: review?(bobbyholley) → review+
> Can't you just pass aCallerType?

Yes.  Good catch; I thought I had updated that.
Attachment #8832019 - Flags: review?(jib) → review+
Attachment #8832018 - Flags: review?(jib) → review+
Comment on attachment 8832023 [details] [diff] [review]
part 20.  Get rid of IsCallerChrome in XSLT

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

r=me

::: dom/xslt/xslt/txMozillaXSLTProcessor.h
@@ +129,5 @@
>          aRv = RemoveParameter(aNamespaceURI, aLocalName);
>      }
>  
> +    uint32_t Flags(mozilla::dom::SystemCallerGuarantee);
> +    void SetFlags(uint32_t aFlags, mozilla::dom::SystemCallerGuarantee);

This is so much nicer than the old system.
Attachment #8832023 - Flags: review?(peterv) → review+

Comment 37

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b13f152004
part 1.  Stop using IsCallerChrome in DOMParser::Constructor.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5a8c8c6be5
part 2.  Stop using IsCallerChrome in CheckerboardReportService::IsEnabled.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/792c2c0560ba
part 3.  Stop using IsCallerChrome in nsContentUtils::IsCutCopyAllowed().  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fab67512801
part 4.  Stop using IsCallerChrome in nsContentUtils::IsRequestFullScreenAllowed.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/01a24ce3398e
part 5.  Stop using IsCallerChrome in nsINode. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/18e645747093
part 6.  Add a SystemCallerGuarantee class that we use for [ChromeOnly, NeedsCallerType] cases.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/897667c66ffc
part 7.  Mostly stop using IsCallerChrome in nsObjectLoadingContent.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6327de6de6
part 8.  Stop using IsCallerChrome in UnwrapArgImpl.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f80b927366
part 9.  Stop using IsCallerChrome in WebGL.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/be91bfa95031
part 10.  Stop using IsCallerChrome in CanvasRenderingContext2D.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9329505877
part 11.  Remove the use of ThreadsafeIsCallerChrome in Event::Init, since that's not really interested in the _caller_ anyway.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d75c539b95ca
part 12.  Remove the use of IsCallerChrome in FetchRequest.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb37a584eab3
part 13.  Remove the use of IsCallerChrome in dom/file.  r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/25880d675650
part 14.  Remove the use of IsCallerChrome in IDBFactory.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d85eb6ac5f
part 15.  Remove the use of IsCallerChrome in ApplyConstraints. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/c307d838a784
part 16.  Remove the use of IsCallerChrome in GetUserMedia.  r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0dcb4003a9
part 17.  Get rid of IsCallerChrome in the quota manager service.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/127a99a30553
part 18.  Get rid of IsCallerChrome in workers.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/65ceed819549
part 19.  Get rid of IsCallerChrome in XMLDocument::Load.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c167d2724d
part 20.  Get rid of IsCallerChrome in XSLT.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/50aec64a8cdd
part 21.  Get rid of some IsCallerChrome usage in XPConnect. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/0681970b9082
part 22.  Get rid of IsCallerChrome in geometry utils.  r=dholbert
You need to log in before you can comment on or make changes to this bug.