Bug 87993 – window.internals is not good for changing persistent settings

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tools / Tests

[Alexey Proskuryakov]

1. Changing a setting in one window.internals.settings object affects all frames in a page, but does not work across secondary windows. This is inconsistent.

2. If a test navigates to a different URL (with waitUntilDone/notifyDone), settings are restored from last document's InternalSettings instead of first one's, so changes made in first one just leak to future tests.

3. Code design is not very clear. Settings are per-page, yet many Internals methods take Document, Frame or a global context.

[Hajime Morrita]

(In reply to comment #0)
> 1. Changing a setting in one window.internals.settings object affects all frames in a page, but does not work across secondary windows. This is inconsistent.
> 
Right.

> 2. If a test navigates to a different URL (with waitUntilDone/notifyDone), settings are restored from last document's InternalSettings instead of first one's, so changes made in first one just leak to future tests.
> 
> 3. Code design is not very clear. Settings are per-page, yet many Internals methods take Document, Frame or a global context.
This is also true. InternalSettings started from simple extraction from 
Internals object, which had no state at that time.
We could have better lifecycle representation around here.

[Jesus Sanchez-Palencia]

Hi! I will try to understand the issue related to this bug a bit further to see if I can help.

(In reply to comment #1)
> (In reply to comment #0)
> > 1. Changing a setting in one window.internals.settings object affects all frames in a page, but does not work across secondary windows. This is inconsistent.
> > 
> Right.

Any ideas? What would be more consistent, not affecting all frames in a page or working across secondary windows?


> 
> > 2. If a test navigates to a different URL (with waitUntilDone/notifyDone), settings are restored from last document's InternalSettings instead of first one's, so changes made in first one just leak to future tests.

Due to m_settings->restoreTo(document->page()->settings()) in Internals::reset(), right? We could fix this by keeping a reference to the first document and then using it to restore the settings, but is this what we want?


> > 
> > 3. Code design is not very clear. Settings are per-page, yet many Internals methods take Document, Frame or a global context.
> This is also true. InternalSettings started from simple extraction from 
> Internals object, which had no state at that time.
> We could have better lifecycle representation around here.

What do you mean by better lifecycle representation more specifically?

[Hajime Morrita]

(In reply to comment #2)
> Hi! I will try to understand the issue related to this bug a bit further to see if I can help.
> 
> (In reply to comment #1)
> > (In reply to comment #0)
> > > 1. Changing a setting in one window.internals.settings object affects all frames in a page, but does not work across secondary windows. This is inconsistent.
> > > 
> > Right.
> 
> Any ideas? What would be more consistent, not affecting all frames in a page or working across secondary windows?
> 
> 
> > 
> > > 2. If a test navigates to a different URL (with waitUntilDone/notifyDone), settings are restored from last document's InternalSettings instead of first one's, so changes made in first one just leak to future tests.
> 
> Due to m_settings->restoreTo(document->page()->settings()) in Internals::reset(), right? We could fix this by keeping a reference to the first document and then using it to restore the settings, but is this what we want?

I don't think changing document lifetime by holding reference is a good idea.
It potentially hides real bugs since there had been man crashes at the end of
document life. We can hold a reference to Internals object though.

> 
> 
> > > 
> > > 3. Code design is not very clear. Settings are per-page, yet many Internals methods take Document, Frame or a global context.
> > This is also true. InternalSettings started from simple extraction from 
> > Internals object, which had no state at that time.
> > We could have better lifecycle representation around here.
> 
> What do you mean by better lifecycle representation more specifically?

My current thinking is to attach Internals object to the Page and
put it for each window, instead of creating Internals for each time.
By doing this, Internals and InternalSettings will have same lifetime as Page.

Does this make sense?

[Tony Chang]

Hi Alexey and Morria,

I was wondering what work remains for this bug.  Replying out of order:

> 2. If a test navigates to a different URL (with waitUntilDone/notifyDone), settings are restored from last document's InternalSettings instead of first one's, so changes made in first one just leak to future tests.

I think this was fixed by Morrita when he made the lifetime of InternalSettings match that of Page in bug 88499.

> 3. Code design is not very clear. Settings are per-page, yet many Internals methods take Document, Frame or a global context.

I moved everything off of internals.settings that doesn't have to do with the Settings class or RuntimeEnabledFeatures.  If you want, I can move all the RuntimeEnabledFeatures to internals.

> 1. Changing a setting in one window.internals.settings object affects all frames in a page, but does not work across secondary windows. This is inconsistent.

I guess this could be confusing, although given that there are only 186 files that call setCanOpenWindows, maybe that's OK?  It seems consistent that changes to internals.settings has the same scope as the Settings class, which is the Page, but maybe that's not obvious to most developers?

I'm not sure how we would solve this short of implementing something like Preferences that will keep all the Settings instances synchronized.  I'm not sure that's worth the code complexity.


Let me know how you would like to see internals and internals.settings improved.

[Hajime Morrita]

Hi Tony,

(In reply to comment #4)
> Hi Alexey and Morria,
> 
> I was wondering what work remains for this bug.  Replying out of order:
I believe this can be closed. The concerns was addressed by Bug 88499.
I should have posted the patch here instead of Bug 88499.

(In reply to comment #0)
> 1. Changing a setting in one window.internals.settings object affects all frames in a page, but does not work across secondary windows. This is inconsistent.
> 

Now internals.settings is a supplement of a Page object.
It just provides a API for Page::settings() This is straightforward I think,
(RuntimeSettings might be tricky though.)


> 2. If a test navigates to a different URL (with waitUntilDone/notifyDone), settings are restored from last document's InternalSettings instead of first one's, so changes made in first one just leak to future tests.
> 

This one was fixed by Bug 88499. The state was reset for each test.

> 3. Code design is not very clear. Settings are per-page, yet many Internals methods take Document, Frame or a global context.

Well, this one could be improved. There are some method which takes a Document
as a first parameter. They could be eliminated. 
I don't think this blocks other bugs though.

[Tony Chang]

I filed bug 107301 for cleaning up methods that take document as a parameter.

*** This bug has been marked as a duplicate of bug 88499 ***