1405372 - Move fullscreen logic out of nsGlobalWindow and into nsDocShell

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1405372 - Part 1: Add a GetDocShell method to nsIDocShellTreeItem,

Review commit: https://reviewboard.mozilla.org/r/186444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186444/

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1405372 - Part 2: Move FullScreen methods and state from nsGlobalWindow into nsDocShell,

Review commit: https://reviewboard.mozilla.org/r/186446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186446/

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Ugh - I decided to try using mozreview for this, as I hoped it would detect that most of the code was just moved from nsGlobalWindow into nsDocShell, and make it easier to review, but it seems its not smart enough to do that.

This won't be able to land until bug 1401379 lands, but I figure I might as well see if I can get this landed. As far as I can tell it passes tests.

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8915208 [details]
Bug 1405372 - Part 2: Move FullScreen methods and state from nsGlobalWindow into nsDocShell,

https://reviewboard.mozilla.org/r/186446/#review191522


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: docshell/base/nsDocShell.cpp:848
(Diff revision 1)
> +  }
> +
> +  NS_IMETHOD Run() override;
> +
> +private:
> +  ~FullscreenTransitionTask() override

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8915207 [details]
Bug 1405372 - Part 1: Add a GetDocShell method to nsIDocShellTreeItem,

https://reviewboard.mozilla.org/r/186444/#review191544

::: docshell/base/nsIDocShellTreeItem.idl:185
(Diff revision 1)
>  	                                      in nsIDocShellTreeItem aRequestor,
>  	                                      in nsIDocShellTreeItem aOriginalRequestor);
>  
>    [noscript,nostdcall,notxpcom] nsIDocument getDocument();
>    [noscript,nostdcall,notxpcom] nsPIDOMWindowOuter getWindow();
> +  [noscript,nostdcall,notxpcom] nsIDocShell getDocShell();

Please document.  In particular, explain when (and whether?) this can return null.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8915208 [details]
Bug 1405372 - Part 2: Move FullScreen methods and state from nsGlobalWindow into nsDocShell,

https://reviewboard.mozilla.org/r/186446/#review191662

It seems to be mostly just code movement, with several helper functions added (duplicated?) I guess this is fine. But I'm not very familiar with DOM and docshell code in general, and not exactly sure what's the meaning of some code changes. Also I'm not a peer in either DOM or docshell, so I cannot review anyway. I'd rather redirect this to smaug.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

It seems smaug doesn't accept review request at the moment... Probably busy on his own patches.

ni? smaug for the review request.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Move FullScreen methods and state from nsGlobalWindow into nsDocShell

olli requested that I post the raw patch.

MozReview-Commit-ID: ITM9wD52kBd

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8915628 [details] [diff] [review]
Part 2: Move FullScreen methods and state from nsGlobalWindow into nsDocShell

>+MakeWidgetFullscreen(nsDocShell* aDocShell, FullscreenReason aReason,
>+                     bool aFullscreen)
>+{
>+  nsCOMPtr<nsIBaseWindow> window = aDocShell->GetTreeOwnerAsWindow();
>+  nsCOMPtr<nsIWidget> widget;
>+  window->GetMainWidget(getter_AddRefs(widget));
I don't see anything guaranteeing that window isn't null. So, please add a null check.

You're inconsistent with -> at the end of a line. Some times it goes to the beginning of the new line, some times it stays in the previous line.
I think I'd prefer
  foo->
   Bar();

In couple of places you use mContentViewer->GetDocument(). Why not mScriptGlobal->GetExtantDoc() to keep exactly the old behavior?


+  // Fullscreen logic
+
+  nsresult SetFullScreen(bool aFullScreen) {
+    return SetFullscreenInternal(FullscreenReason::ForFullscreenMode, aFullScreen);
+  }
{ goes to its own line.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

I'm taking a different approach with the nsGlobalWindow split, so marking this wontfix.