Closed Bug 1472087 Opened 6 years ago Closed 6 years ago

DeCOMtaminate nsIDocShellLoadInfo

Categories

(Core :: DOM: Navigation, enhancement, P2)

Product:
Core
Component:
DOM: Navigation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

Bug 1472087 - deCOMtaminate nsIDocShellLoadInfo;
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1472087 - Remove nsIDocShellLoadInfo.idl;
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1472087 - Remove nsDocShell::CreateLoadInfo;
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1472087 - Convert nsDocShellLoadInfo to WebIDL style;
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1472087 - Remove nsDocShellLoadInfo::LoadTypes;
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1472087 - Readd comments from nsIDocShellLoadInfo;
59 bytes, text/x-review-board-request
nika
: review+
Details 
nsIDocShellLoadInfo isn't really used between JS and C++ (the only IDL references are as parameters, and the only JS use is of the loadHistory type), so we should be able to consolidate it to C++ only.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b558e4dcdd75e98e75ebb1a79795566bc54fbe38

Cleanup left:
- Turn nsDocShellInfoLoadType into its own enum in nsDocShellLoadTypes
- Turn nsDocShellInfoReferrerPolicy into its own enum in nsDocShellLoadTypes
- Change nsDocShellLoadInfo getters/setters to webidl style instead of XPCOM style
Priority: -- → P2
Comment on attachment 8989506 [details]
Bug ? - Remove redundant Docshell destroyed check;

Wrong patch, not supposed to be in this set.
Attachment #8989506 - Attachment is obsolete: true
Attachment #8989506 - Flags: review?(nika)
Attachment #8989506 - Attachment is obsolete: false
Attachment #8989506 - Attachment is obsolete: true
Comment on attachment 8988970 [details]
Bug 1472087 - deCOMtaminate nsIDocShellLoadInfo;

https://reviewboard.mozilla.org/r/254084/#review261634

::: docshell/base/nsDocShell.cpp:713
(Diff revision 2)
>    if (aLoadInfo) {
>      aLoadInfo->GetReferrer(getter_AddRefs(referrer));
>      aLoadInfo->GetOriginalURI(getter_AddRefs(originalURI));
>      GetMaybeResultPrincipalURI(aLoadInfo, resultPrincipalURI);
>      aLoadInfo->GetLoadReplace(&loadReplace);
> -    nsDocShellInfoLoadType lt = nsIDocShellLoadInfo::loadNormal;
> +    nsDocShellLoadInfo::nsDocShellInfoLoadType lt = nsDocShellLoadInfo::loadNormal;

I think that, given this type is namespaced now, we can probably get away with calling it just 'LoadType'.

::: docshell/base/nsDocShellLoadInfo.h:23
(Diff revision 2)
>  
> -class nsDocShellLoadInfo : public nsIDocShellLoadInfo
> +class nsDocShellLoadInfo
>  {
>  public:
> +  typedef uint32_t nsDocShellInfoLoadType;
> +  typedef uint32_t nsDocShellInfoReferrerPolicy;

both of these can probably be given more terse names, but come to think of it you might do that in one of the other... 6? patches.

::: toolkit/components/viewsource/content/viewSource-content.js:268
(Diff revision 2)
>                      .createInstance(Ci.nsISHEntry);
>      shEntry.setURI(Services.io.newURI(viewSrcURL));
>      shEntry.setTitle(viewSrcURL);
>      let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
>      shEntry.triggeringPrincipal = systemPrincipal;
> -    shEntry.loadType = Ci.nsIDocShellLoadInfo.loadHistory;
> +    shEntry.loadType = 2; /* nsDocShellLoadInfo::loadHistory */

:-/ - Thanks for adding the comment there at least!

::: toolkit/modules/sessionstore/SessionHistory.jsm:346
(Diff revision 2)
>  
>      shEntry.setURI(Utils.makeURI(entry.url));
>      shEntry.setTitle(entry.title || entry.url);
>      if (entry.subframe)
>        shEntry.setIsSubFrame(entry.subframe || false);
> -    shEntry.loadType = Ci.nsIDocShellLoadInfo.loadHistory;
> +    shEntry.loadType = 2; /* nsDocShellLoadInfo::loadHistory */

:-/
Attachment #8988970 - Flags: review?(nika) → review+
Comment on attachment 8988971 [details]
Bug 1472087 - Remove nsIDocShellLoadInfo.idl;

https://reviewboard.mozilla.org/r/254086/#review261636
Attachment #8988971 - Flags: review?(nika) → review+
Comment on attachment 8988972 [details]
Bug 1472087 - Remove nsDocShell::CreateLoadInfo;

https://reviewboard.mozilla.org/r/254088/#review261638
Attachment #8988972 - Flags: review?(nika) → review+
Comment on attachment 8989507 [details]
Bug 1472087 - Convert nsDocShellLoadInfo to WebIDL style;

https://reviewboard.mozilla.org/r/254542/#review261640

::: commit-message-1b750:4
(Diff revision 1)
> +Bug 1472087 - Convert nsDocShellLoadInfo to WebIDL style; r?nika
> +
> +While nsDocShellLoadInfo isn't represented by WebIDL (because we don't
> +need it in JS currently), make the getter/setter interface look

Is the plan to eventually also expose nsDocShellLoadInfo to JS so that we can convert all load entry points in nsDocShell to using it?

::: docshell/base/nsDocShellLoadInfo.h:22
(Diff revision 1)
>  class nsIDocShell;
>  
>  class nsDocShellLoadInfo
>  {
>  public:
>    typedef uint32_t nsDocShellInfoLoadType;

Can we kill this typedef? We have an enum type now.

::: docshell/base/nsDocShellLoadInfo.h:157
(Diff revision 1)
>    bool mInheritPrincipal;
>    bool mPrincipalIsExplicit;
>    bool mForceAllowDataURI;
>    bool mOriginalFrameSrc;
>    bool mSendReferrer;
> -  nsDocShellInfoReferrerPolicy mReferrerPolicy;
> +  uint32_t mReferrerPolicy;

Could/should we use the 'mozilla::net::ReferrerPolicy' enum for this to make it more clear?

::: docshell/base/nsDocShellLoadInfo.cpp:159
(Diff revision 1)
>  {
>    mOriginalFrameSrc = aOriginalFrameSrc;
> -  return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> +nsDocShellLoadInfo::nsDocShellInfoLoadType

Can we make this nsDocShellLoadInfo::LoadType now - we have the enum :-)

::: docshell/base/nsDocShellLoadInfo.cpp:166
(Diff revision 1)
> -  *aLoadType = mLoadType;
> -  return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> -nsDocShellLoadInfo::SetLoadType(nsDocShellInfoLoadType aLoadType)
> +void
> +nsDocShellLoadInfo::SetLoadType(nsDocShellLoadInfo::nsDocShellInfoLoadType aLoadType)

Same here
Attachment #8989507 - Flags: review?(nika) → review+
Comment on attachment 8989508 [details]
Bug 1472087 - Remove nsDocShellLoadInfo::LoadTypes;

https://reviewboard.mozilla.org/r/254544/#review261646

::: toolkit/components/viewsource/content/viewSource-content.js:268
(Diff revision 1)
>                      .createInstance(Ci.nsISHEntry);
>      shEntry.setURI(Services.io.newURI(viewSrcURL));
>      shEntry.setTitle(viewSrcURL);
>      let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
>      shEntry.triggeringPrincipal = systemPrincipal;
> -    shEntry.loadType = 2; /* nsDocShellLoadInfo::loadHistory */
> +    shEntry.loadType = 4; /* nsDocShellLoadTypes LOAD_HISTORY */

This is a touch more nasty now :-/...

Maybe even add a method to nsISHistory like setAsHistoryLoad()? D:
Attachment #8989508 - Flags: review?(nika) → review+
Comment on attachment 8989509 [details]
Bug 1472087 - Readd comments from nsIDocShellLoadInfo;

https://reviewboard.mozilla.org/r/254546/#review261648

::: docshell/base/nsDocShellLoadInfo.h:21
(Diff revision 1)
>  class nsISHEntry;
>  class nsIURI;
>  class nsIDocShell;
>  
> +/**
> + * nsDocShellLoadInfo defines an interface for specifying setup information used

nsDocShellLoadInfo isn't really an interface anymore :-P
Attachment #8989509 - Flags: review?(nika) → review+
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9df2dcfe09f
deCOMtaminate nsIDocShellLoadInfo; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffbed43e8c35
Remove nsIDocShellLoadInfo.idl; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c808bf7da8
Remove nsDocShell::CreateLoadInfo; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0289eb6a02d
Convert nsDocShellLoadInfo to WebIDL style; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd86e6f12106
Remove nsDocShellLoadInfo::LoadTypes; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9d71567046
Readd comments from nsIDocShellLoadInfo; r=nika
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: