1137009 - Video fullscreen confuses itself with F11 fullscreen state

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

STR:

* Navigate to YouTube video
* Click on Fullscreen on video
* Close window with ctrl-F4
* Start Firefox again

Expected results:
Firefox returns to maximised or windowed mode

Actual results:

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Firefox is in F11 fullscreen mode.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Xidorn - is this issue on your radar?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

No. Thanks for reminding me.

I think we have various issues when a page is closed in fullscreen state.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

On Mac, the window state is also confused if Firefox is exited via Cmd-Q when on video fullscreen.

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Jet - can I vote for this to happen sooner rather than later? It's actually quite annoying - especially if Firefox crashes when you're watching a video in full screen. A lot of people wouldn't know to press F11 to fix the issue.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Actually the STR is not that precise. It may need time between entering fullscreen and closing the window to reproduce this issue.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

This patch tries to reset the "persist" attribute on the topmost <window> element for DOM Fullscreen to avoid persisting the window state, and restore that when exiting fullscreen.

On Windows, the window would still open maximized in case Firefox unexpectedly exits in DOM Fullscreen. It may need further investigation, but has been far less annoying.

Comment 8

[Jan Varga [:janv]]

Comment on attachment 8647979 [details] [diff] [review]
patch

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

Some comments may not apply if you rework the patch, especially if you eliminate the backup attribute.

::: dom/base/nsGlobalWindow.cpp
@@ +6548,5 @@
> +    }
> +    // We don't want to persist the window state if we enter fullscreen
> +    // for Fullscreen API, because we should not, and actually we cannot
> +    // properly restore to that state. For this reason, we need disable
> +    // persistence and restore it accordingly.

This comment should probably be merged with the comment above.

@@ +6549,5 @@
> +    // We don't want to persist the window state if we enter fullscreen
> +    // for Fullscreen API, because we should not, and actually we cannot
> +    // properly restore to that state. For this reason, we need disable
> +    // persistence and restore it accordingly.
> +    if (!aFullScreen) {

You are checking the same variable twice. Actually it's a bit hard to understand this logic, one piece of the logic lives here and the rest in nsXULWindow.

@@ +6550,5 @@
> +    // for Fullscreen API, because we should not, and actually we cannot
> +    // properly restore to that state. For this reason, we need disable
> +    // persistence and restore it accordingly.
> +    if (!aFullScreen) {
> +      xulWin->ToggleAttributePersistence(true);

I would at least modify this to |/* aAllowPersist */ true| here and below.
Also, don't you need to check |aReason == eForFullscreenAP)| here too ?

::: xpfe/appshell/nsXULWindow.cpp
@@ +1540,5 @@
> +MoveElementAttributeValue(Element* aElement, const nsAString& aFromAttr,
> +                          const nsAString& aToAttr)
> +{
> +  ErrorResult rv;
> +  nsAutoString value;

No need for nsAutoString AFAIK. You don't need to modify that string.

@@ +1547,5 @@
> +  aElement->SetAttribute(aToAttr, value, rv);
> +}
> +
> +NS_IMETHODIMP
> +nsXULWindow::ToggleAttributePersistence(bool aAllowPersist)

Why do we need to backup the "persist" attribute. Could we instead set a flag that we don't want to persist attributes if we are in dom full screen mode ?

@@ +1550,5 @@
> +NS_IMETHODIMP
> +nsXULWindow::ToggleAttributePersistence(bool aAllowPersist)
> +{
> +  nsCOMPtr<dom::Element> element = GetWindowDOMElement();
> +  if (!element)

if () {
}

@@ +1553,5 @@
> +  nsCOMPtr<dom::Element> element = GetWindowDOMElement();
> +  if (!element)
> +    return NS_ERROR_FAILURE;
> +
> +  static NS_NAMED_LITERAL_STRING(kPersistBackupAttribute, "_persist");

Maybe move (and rename) this to the list of other attributes (right after #include section) ?

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 10

[Jan Varga [:janv]]

Comment on attachment 8649684 [details] [diff] [review]
patch

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

Much much better, thanks!

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

We probably shouldn't persist fullscreen state at all, no matter it is for DOM fullscreen or fullscreen mode, because we always apply that state to the next window we create, which could be confusing and problematic.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

Bug 1137009 part 1 - Do not persist xul window attributes when they change.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen.

Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Jan, could you please review these new patches?

Comment 15

[Jan Varga [:janv]]

Comment on attachment 8650792 [details]
MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

https://reviewboard.mozilla.org/r/16691/#review15999

::: dom/xul/XULDocument.cpp:1002
(Diff revision 1)
> +    if (!aElement->IsXULElement(nsGkAtoms::window)) {

So here you are just avoiding to persist attributes for all window elements and the other patch checks the full screen state. Is that correct ?

Comment 16

[Jan Varga [:janv]]

Comment on attachment 8650793 [details]
MozReview Request: Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen.

https://reviewboard.mozilla.org/r/16693/#review15991

::: xpfe/appshell/nsXULWindow.cpp:1438
(Diff revision 1)
> +  if (nsPIDOMWindow* domWindow = mDocShell->GetWindow()) {

This entire blog could be moved before GetWindowDOMElement() call I think.
Also, instead of calling |mDocShell->GetWindow()| you could use GetWindowDOMWindow() which is used elsewhere.

Comment 17

[Jan Varga [:janv]]

s/blog/block

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jan Varga [:janv] from comment #15)
> Comment on attachment 8650792 [details]
> MozReview Request: Bug 1137009 part 1 - Do not persist xul window attributes
> when they change.
> 
> https://reviewboard.mozilla.org/r/16691/#review15999
> 
> ::: dom/xul/XULDocument.cpp:1002
> (Diff revision 1)
> > +    if (!aElement->IsXULElement(nsGkAtoms::window)) {
> 
> So here you are just avoiding to persist attributes for all window elements
> and the other patch checks the full screen state. Is that correct ?

Yes, exactly. So is there any concern about this patch?

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8650792 [details]
MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

Bug 1137009 part 1 - Do not persist xul window attributes when they change.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8650793 [details]
MozReview Request: Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen.

Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen.

Comment 21

[Jan Varga [:janv]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #18)
> (In reply to Jan Varga [:janv] from comment #15)
> > Comment on attachment 8650792 [details]
> > MozReview Request: Bug 1137009 part 1 - Do not persist xul window attributes
> > when they change.
> > 
> > https://reviewboard.mozilla.org/r/16691/#review15999
> > 
> > ::: dom/xul/XULDocument.cpp:1002
> > (Diff revision 1)
> > > +    if (!aElement->IsXULElement(nsGkAtoms::window)) {
> > 
> > So here you are just avoiding to persist attributes for all window elements
> > and the other patch checks the full screen state. Is that correct ?
> 
> Yes, exactly. So is there any concern about this patch?

So, if I understand it correctly, you could have just one check in XULDocument::Persist() to avoid persisting attributes if we are in full screen mode. You have access to nsPIDOMWindow through mDocumentContainer, see XULDocument::GetWindowRoot().
However, it seems you decided to do it earlier. In that case, nsXULWindow::SavePersistentAttributes() changes look good, but I don't understand why you only check |aElement->IsXULElement(nsGkAtoms::window| in XULDocument::AttributeChanged(). You should check if we are in full screen mode, no ?
Otherwise, one would expect that attributes are correctly persisted when the "persist" attribute changes on the "window" element.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jan Varga [:janv] from comment #21)
> Otherwise, one would expect that attributes are correctly persisted when the
> "persist" attribute changes on the "window" element.

Hmm, that probably makes sense if the attributes may be changed in ways other than the related methods of nsXULWindow. OK, I'll update the patch. It should be in one single patch then.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8650792 [details]
MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

Comment 24

[Jan Varga [:janv]]

Comment on attachment 8650792 [details]
MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

https://reviewboard.mozilla.org/r/16691/#review16541

r=me

Comment 25

[Jan Varga [:janv]]

Comment on attachment 8650792 [details]
MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

https://reviewboard.mozilla.org/r/16691/#review16547

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/873acda091b7

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Backout for bc test failure
https://hg.mozilla.org/integration/mozilla-inbound/rev/76826fe33431

I'm pretty confused what is the "persist" attribute supposed to do. Do we ever change that attribute on <window>? Do we ever want the window not to set the attributes when the window state changes?

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4951f7a8b47f

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

The attributes should eventually be saved via the AttributeChanged callback in
XULDocument if one is specified in the "persist" attribute.

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Sorry for requesting review again. This patchset now works fine on the try server.

Comment 32

[[e-mail]]

(In reply to Xidorn Quan from comment #27)
> I'm pretty confused what is the "persist" attribute supposed to do.
It's basically a convenient storage system for attribute values.

> Do we ever change that attribute on <window>?
It's rare that we change the "persist" attribute's value itself.

> Do we ever want the window not to set the attributes when the window state changes?
It was probably originally done as an optimisation for windows which don't persist.

Comment 33

[Jan Varga [:janv]]

Comment on attachment 8658550 [details]
MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

https://reviewboard.mozilla.org/r/18645/#review17207

I think someone else should take a look at this.

Comment 34

[Jan Varga [:janv]]

Comment on attachment 8658551 [details]
MozReview Request: Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

https://reviewboard.mozilla.org/r/18647/#review17209

I think someone else should take a look at this.

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8658550 [details]
MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

The attributes should eventually be saved via the AttributeChanged callback in
XULDocument if one is specified in the "persist" attribute.

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8658551 [details]
MozReview Request: Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

Comment 37

[Neil Deakin]

Comment on attachment 8658550 [details]
MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

Seems ok, but why do you need to remove the optimization?

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Neil Deakin from comment #37)
> Comment on attachment 8658550 [details]
> MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes
> but do not persist them in nsXULWindow::SavePersistentAttributes() directly.
> 
> Seems ok, but why do you need to remove the optimization?

Do you mean the early return when "persist" attribute is empty? This removal is probably not necessary, but I think removing that matches the new name. I hope to handle all persistence in one place (in XULDocument::AttributeChanged()), so nsXULWindow::SavePersistentAttributes() no longer handles persistence directly, and thus it is weird to check "persist" attribute there.

And is it common to have that attribute clear for optimization? It also doesn't seem correct to me that an attribute is not set at all merely because it is not listed in "persist".

Comment 39

[Neil Deakin]

I meant you removed the checks that don't assign the attributes when they don't exist in the persist list. The attributes are only used for the benefit of being persisted, so this patch adds these attributes unconditionally, including zorder which really isn't used at all.

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

I don't think the attributes are only used for being persisted. At least some tests check sizemode attribute for the current window state. See the failures here: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e9cb31373ec2 for the previous patchset, which shows what would happen if I do not set the attribute properly.

If zorder is never used, and the attributes are only for being persisted, is there any reason why we shouldn't just remove that attribute?

Comment 41

[Neil Deakin]

Comment on attachment 8658550 [details]
MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

> If zorder is never used, and the attributes are only for being persisted, is
> there any reason why we shouldn't just remove that attribute?

It looks like the zorder is only used for the help window in Seamonkey, but I don't think it is really needed, so I think we could just remove it.

With that change, I think this patch is probably ok then. Would an alternative option be to have ShouldPersistAttribute explicitly look for the width/height/screenX/screenY attributes and not the other attributes and not have this patch?

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Neil Deakin from comment #41)
> Comment on attachment 8658550 [details]
> MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes
> but do not persist them in nsXULWindow::SavePersistentAttributes() directly.
> 
> > If zorder is never used, and the attributes are only for being persisted, is
> > there any reason why we shouldn't just remove that attribute?
> 
> It looks like the zorder is only used for the help window in Seamonkey, but
> I don't think it is really needed, so I think we could just remove it.
> 
> With that change, I think this patch is probably ok then. Would an
> alternative option be to have ShouldPersistAttribute explicitly look for the
> width/height/screenX/screenY attributes and not the other attributes and not
> have this patch?

The main motivation for this patch is to remove the explicit persisting in SavePersistentAttributes(), because XULDocument::AttributeChanged() will do that anyway. I don't want to duplicate the same logic in two places, especially given the logic in SavePersistentAttributes() is not seems correct to me.

Thus I don't think doing any change in ShouldPersistAttribute() could remove the necessity of this patch.
We have to do something in SavePersistentAttributes() anyway.

Also, I feel that removing zlevel attribute is not directly related to this bug. Would you mind me landing this patchset as-is, and do the removal in a separate bug?

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

s/is not seems/does not seem

Comment 44

[Neil Deakin]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #42)
> The main motivation for this patch is to remove the explicit persisting in
> SavePersistentAttributes(), because XULDocument::AttributeChanged() will do
> that anyway. I don't want to duplicate the same logic in two places,
> especially given the logic in SavePersistentAttributes() is not seems
> correct to me.

Yes, that part is good.

> Also, I feel that removing zlevel attribute is not directly related to this
> bug. Would you mind me landing this patchset as-is, and do the removal in a
> separate bug?

OK.

Comment 45

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5322e5fa07efb3f62c6383c4a0bfa5ef588363
Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c77e4edc2a55facff5faa192180aa7b6d887129
Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen. r=enndeakin

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Neil Deakin from comment #44)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #42)
> > Also, I feel that removing zlevel attribute is not directly related to this
> > bug. Would you mind me landing this patchset as-is, and do the removal in a
> > separate bug?
> 
> OK.

Filed bug 1207883.

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a5322e5fa07
https://hg.mozilla.org/mozilla-central/rev/2c77e4edc2a5

Comment 48

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8658550 [details]
MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

Approval Request Comment
[Feature/regressing bug #]: probably not a regression
[User impact if declined]: meet various issues when using Fullscreen Mode or Fullscreen API, e.g. this bug, bug 946768, bug 1197416, and bug 1204332
[Describe test coverage new/current, TreeHerder]: not aware of any
[Risks and why]: have moderate risk that window may not restore to the proper state after restart the browser if there is any case I did not catch
[String/UUID change made/needed]: n/a

Comment 49

[Xidorn Quan [:xidorn] UTC+10]

Actually for bug 946768, it is a regression since bug 1173930. For other bugs, I'm not aware of any bug caused them.

Comment 51

[Loic]

Is it normal after this bugfix that Nightly fails to restart in FS mode if it has been closed in FS mode previously?

Comment 52

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Loic from comment #51)
> Is it normal after this bugfix that Nightly fails to restart in FS mode if
> it has been closed in FS mode previously?

Yes, it is intended, because starting in fullscreen mode causes issues on at least OS X 10.7+ (e.g. bug 1207674) and Windows 10 (bug 1204332).

Comment 53

[Loic]

Is it a permanent change?

Comment 54

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Loic from comment #53)
> Is it a permanent change?

Likely. At least I have no plan to reverse this change.

If you think we should support restroing to fs mode, you may file a bug. But I don't think that's important to fix.

AFAICS, currently only IE restores to fs mode, and that isn't even true for Edge which doesn't seem to have fs mode at all.

Comment 56

[Jet Villegas (inactive)]

[Tracking Requested - why for this release]: see comment 48.

Comment 57

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8658550 [details]
MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly.

OK, let's uplift this, and keep an eye open for fullscreen or window attribute issues in nightly and aurora.

Comment 58

[Carsten Book [:Tomcat]]

do we want to uplift both parts or just part 1 ?

Comment 59

[Xidorn Quan [:xidorn] UTC+10]

Part 1 on its own doesn't fix this issue. We should uplift both parts.

Comment 60

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8658551 [details]
MozReview Request: Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

Let's not forget part 2 for aurora uplift

Comment 61

[Liz Henry (:lizzard) (relman/hg->git project)]

Sorry, I was in transit and got interrupted :)

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e06450a934c
https://hg.mozilla.org/releases/mozilla-aurora/rev/735ebc504963

Comment 64

[Jorg K (CEST = GMT+2)]

Would you like to back this out since it's causing regressions, see bug 1211344?
(Also see: Bug 1211344 comment #10).

We don't know which other XUL panels are affected, maybe in some add-ons.
I think the rule is: If it causes regressions, it gets backed out, at least from Aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e06450a934c
https://hg.mozilla.org/releases/mozilla-aurora/rev/735ebc504963

https://hg.mozilla.org/mozilla-central/rev/7a5322e5fa07
https://hg.mozilla.org/mozilla-central/rev/2c77e4edc2a5

Comment 65

[Xidorn Quan [:xidorn] UTC+10]

Before we actually back out this bug, we should not reopen it.

Making this bug depends on the regression bug is enough for tracking.

Comment 66

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, so I gather you want to leave this on aurora and fix the regression in bug 1211344.  Is this a better approach for this particular set of changes? Or is it something you could fix by backing out the patches here and reworking them?

Comment 67

[Xidorn Quan [:xidorn] UTC+10]

I'll submit a patch in bug 1211344 later today which I believe should be upliftable to aurora.

Comment 68

[Xidorn Quan [:xidorn] UTC+10]

Please backout this changeset in central and aurora. Also note that, backing out this bug would need backing out bug 1211344 first.

Comment 69

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Both patches backed out in 
https://hg.mozilla.org/integration/mozilla-inbound/rev/726a356eef02
https://hg.mozilla.org/releases/mozilla-aurora/rev/38cffa3439e4

Unsure exactly what the flags should be set back to, hopefully this is right. Leaving 1211344 closed since it'll be dealt with here.

Comment 70

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen.

Bug 1137009 - Do not persist xul window attributes when in fullscreen.

Comment 71

[Xidorn Quan [:xidorn] UTC+10]

It should be less risky this time...

Comment 72

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0414eb77ae2c

Comment 73

[Neil Deakin]

Comment on attachment 8674614 [details]
MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen.

> if (!aElement->IsXULElement(nsGkAtoms::window) && !persist.IsEmpty()) {

What if someone wants to persist an attribute that isn't one that XULWindow handles?

Comment 74

[Xidorn Quan [:xidorn] UTC+10]

That attribute would never be persisted. But do we really have code does that?

If that really causes any regression later, I can extend the condition here to exclude only the attributes we handle in XULWindow. If you do think we should be careful about this now, I can do that in this bug directly as well.

Comment 75

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8674614 [details]
MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen.

Bug 1137009 - Do not persist xul window attributes when in fullscreen.

Comment 76

[Xidorn Quan [:xidorn] UTC+10]

OK, now I do exclude only the attributes we do handle in XULWindow.

Comment 77

[Neil Deakin]

Comment on attachment 8674614 [details]
MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen.

I don't know if there's any other attributes that get used for window, but it would good to be safe considering that this bug has had some regressions already.

Comment 78

[Xidorn Quan [:xidorn] UTC+10]

Yeah, I agree.

Comment 79

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/437b3c3ef56c48b25e432f8fd61afdad2c605b86
Bug 1137009 - Do not persist xul window attributes when in fullscreen. r=enndeakin

Comment 80

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/437b3c3ef56c

Comment 81

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8674614 [details]
MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen.

Approval Request Comment
[Feature/regressing bug #]: probably not a regression
[User impact if declined]: meet various issues when using Fullscreen Mode or Fullscreen API, e.g. this bug, bug 946768, bug 1197416, and bug 1204332
[Describe test coverage new/current, TreeHerder]: not aware of any
[Risks and why]: relatively low risk, because we tried harder to avoid changing unrelated behavior in this new patch
[String/UUID change made/needed]: n/a

Comment 82

[Jorg K (CEST = GMT+2)]

FWIW: The issue reported in bug 1211344 is *not* being observed with this new patch, so it's all good. I trust you tested bug 1214064 (which will also receive a test).

Comment 83

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8674614 [details]
MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen.

OK, let's try uplift one more time. But, if this doesn't work, during the last week of aurora (next week) we might want to just keep this to ride on 44.

Comment 84

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #83)
> Comment on attachment 8674614 [details]
> MozReview Request: Bug 1137009 - Do not persist xul window attributes when
> in fullscreen.
> 
> OK, let's try uplift one more time. But, if this doesn't work, during the
> last week of aurora (next week) we might want to just keep this to ride on
> 44.

Thanks.

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e74341409f65

Comment 86

[Saheda Reza [:Antora]]

I have successfully reproduced this bug on Firefox nightly version 33.0a1 according to (2015-02-25) 

It's fixed and verified on Firefox Beta and Latest Nightly 45.0a1
Firefox Beta
Build ID 	20151112144305
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0

Tested OS-32bitWindows 8.1

Comment 87

[Saheda Reza [:Antora]]

It's fixed and verified ?On Latest Developer Edition
(In reply to Saheda Reza [:Antora] from comment #86)
> I have successfully reproduced this bug on Firefox nightly version 33.0a1
> according to (2015-02-25) 
> 
> It's fixed and verified on Firefox Beta and Latest Developer Edition
> Firefox Beta
> Build ID 	20151112144305
> User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0
> 
Latest Developer Edition
Build ID 	20151113004115
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0

> Tested OS-32bitWindows 8.1