We need to hook frame lifetime to make sure the settings coming back to the original value.
Created attachment 145232 [details] Patch
Comment on attachment 145232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145232&action=review > Source/WebCore/testing/InternalSettings.cpp:115 > + if (Page* page = m_frame->page()) > + restoreTo(page->settings()); nit: We have InternalsSettings::settings(). if (Settings* settings = settings()) restoreTo(settings); BTW, is it always ok to restore the settings? For example, 1. An internals object is created for the main frame. (Inernals-A) 2. A test HTML contains an iframe. 3. The iframe is loaded. An internals object for the iframe content is created (Internals-B). The current page settings are saved. 4. Internals-A changes some settings. 5. The iframe is deleted. 6. Internals-B::willDetachPage() is called. The settings saved in step 3 are restored. -> The setting change by Internals is lost. > Source/WebCore/testing/InternalSettings.h:50 > + virtual void willDetachPage() OVERRIDE; Should FrameDestructionObserver::willDetach be pure virtual?
Created attachment 145236 [details] Patch
(In reply to comment #2) > (From update of attachment 145232 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145232&action=review > > > Source/WebCore/testing/InternalSettings.cpp:115 > > + if (Page* page = m_frame->page()) > > + restoreTo(page->settings()); > > nit: We have InternalsSettings::settings(). Yeah, let me use it. > if (Settings* settings = settings()) > restoreTo(settings); > > BTW, is it always ok to restore the settings? For example, > 1. An internals object is created for the main frame. (Inernals-A) > 2. A test HTML contains an iframe. > 3. The iframe is loaded. An internals object for the iframe content is created (Internals-B). The current page settings are saved. > 4. Internals-A changes some settings. > 5. The iframe is deleted. > 6. Internals-B::willDetachPage() is called. The settings saved in step 3 are restored. > -> The setting change by Internals is lost. Good point. Update patch limited the restore operation only for main frames. > > > Source/WebCore/testing/InternalSettings.h:50 > > + virtual void willDetachPage() OVERRIDE; > > Should FrameDestructionObserver::willDetach be pure virtual? Hmm sounds verbose since only a few actually implement it. The updated patch let subclass call super class instead.
Comment on attachment 145236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145236&action=review > Source/WebCore/testing/InternalSettings.cpp:118 > + if (Page* page = m_frame->page()) { > + if (page->mainFrame() == m_frame) > + restoreTo(settings()); > + } > + If an Internals object for a sub-frame changes a setting, it won't be reset, right?
Is this part of bug 87993? I think that you're on the right track overall, however it would be even better to start getting rid of any frame references in InternalSettings, because settings are per-page, and don't really have anything to do with frames.
We shouldn't make multiple InternalSettings objects in one Page.
(In reply to comment #6) > Is this part of bug 87993? > > I think that you're on the right track overall, however it would be even better to start getting rid of any frame references in InternalSettings, because settings are per-page, and don't really have anything to do with frames. Right. I wasn't aware of Bug 87993. But these are related. And making IntenalSettings per-Page sounds right direction. > We shouldn't make multiple InternalSettings objects in one Page. Yeah, we currently don't have good place to do this. But we could have. Lee me thing. Maybe I should start from Bug 87933 and come back here after that.
Does this bug still exist? Since InternalSettings' lifetime is the same as Page, I don't think this is a problem.
Right. closing.