Eliminate chrome::IsTrustedWindowWithScheme

Eliminate chrome::IsTrustedWindowWithScheme

In hindsight, attempting to generalize the settings window behavior was a mistake. This should be simpler and more maintainable.

BUG=417633

[stevenjb, 2014-09-25 20:13:54]

stevenjb@chromium.org changed reviewers:
+ skuhne@chromium.org, sky@chromium.org

[Mr4D (OOO till 08-26), 2014-09-25 21:18:34]

Please see comments. lgtm.

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
File chrome/browser/ui/settings_window_manager.cc (right):

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
chrome/browser/ui/settings_window_manager.cc:68: observers_,
OnNewSettingsWindow(navigate_params_->browser));
At this time you could destroy the navigate_params_ again.

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
chrome/browser/ui/settings_window_manager.cc:80: // is the browser we most
recently navigate to.
.. most recently navigated to.

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
chrome/browser/ui/settings_window_manager.cc:81: if (navigate_params_ &&
navigate_params_->browser == browser)
Don't really like this, but I do not see a good other way to do this either. [A
window property wouldn't help here, and looking for the requested GURL isn't
better either].

[stevenjb, 2014-09-25 21:36:58]

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
File chrome/browser/ui/settings_window_manager.cc (right):

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
chrome/browser/ui/settings_window_manager.cc:68: observers_,
OnNewSettingsWindow(navigate_params_->browser));
On 2014/09/25 21:18:34, Mr4D wrote:
> At this time you could destroy the navigate_params_ again.

Good suggestion now that I think about it, will do.

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
chrome/browser/ui/settings_window_manager.cc:80: // is the browser we most
recently navigate to.
On 2014/09/25 21:18:34, Mr4D wrote:
> .. most recently navigated to.

Done.

https://codereview.chromium.org/595213005/diff/20001/chrome/browser/ui/settin...
chrome/browser/ui/settings_window_manager.cc:81: if (navigate_params_ &&
navigate_params_->browser == browser)
On 2014/09/25 21:18:34, Mr4D wrote:
> Don't really like this, but I do not see a good other way to do this either.
[A
> window property wouldn't help here, and looking for the requested GURL isn't
> better either].

Right, pretty much the same conclusion I came to. I do like resetting
navigate_params_ after Navigate completes and settings_session_map_ is set
however.

[sky, 2014-09-25 23:36:52]

https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
File chrome/browser/ui/settings_window_manager.cc (right):

https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
chrome/browser/ui/settings_window_manager.cc:84: if (navigate_params_ &&
navigate_params_->browser == browser)
Dependencies like this are fragile and result in NavigateParams needing to talk
to more things. I would rather you add something to NavigateParams that
explicitly tells it the browser is a settings browser.

[stevenjb, 2014-09-26 16:39:41]

On 2014/09/25 23:36:52, sky wrote:
>
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
> File chrome/browser/ui/settings_window_manager.cc (right):
> 
>
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
> chrome/browser/ui/settings_window_manager.cc:84: if (navigate_params_ &&
> navigate_params_->browser == browser)
> Dependencies like this are fragile and result in NavigateParams needing to
talk
> to more things. I would rather you add something to NavigateParams that
> explicitly tells it the browser is a settings browser.

I'm not sure that I follow your suggestion. We rely on NavigateParams->browser
for the Navigate() result, so it does not feel like we are adding any
dependencies here to the SettingsWindow code. Tacking on an extra parameter to
NavigateParams just for this seems like adding an unnecessary dependency to
NavigateParams. Or are you suggesting that the Browser should know that it is a
settings browser and we should use something like browser->IsSettingsBrowser()
instead? I was trying to avoid tacking on yet another Browser property.

[sky, 2014-09-26 19:44:48]

What I dislike is having to maintain state assuming that
IsSettingsBrowser is going to be invoked. This makes for awkward
members in SettingsWindowManager. Can't that state be passed into
whoever needs it?

On Fri, Sep 26, 2014 at 9:39 AM,  <stevenjb@chromium.org> wrote:
> On 2014/09/25 23:36:52, sky wrote:
>
>
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
>>
>> File chrome/browser/ui/settings_window_manager.cc (right):
>
>
>
>
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
>>
>> chrome/browser/ui/settings_window_manager.cc:84: if (navigate_params_ &&
>> navigate_params_->browser == browser)
>> Dependencies like this are fragile and result in NavigateParams needing to
>
> talk
>>
>> to more things. I would rather you add something to NavigateParams that
>> explicitly tells it the browser is a settings browser.
>
>
> I'm not sure that I follow your suggestion. We rely on
> NavigateParams->browser
> for the Navigate() result, so it does not feel like we are adding any
> dependencies here to the SettingsWindow code. Tacking on an extra parameter
> to
> NavigateParams just for this seems like adding an unnecessary dependency to
> NavigateParams. Or are you suggesting that the Browser should know that it
> is a
> settings browser and we should use something like
> browser->IsSettingsBrowser()
> instead? I was trying to avoid tacking on yet another Browser property.
>
>
>
> https://codereview.chromium.org/595213005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2014-09-26 20:15:01]

I don't think that SettingsWindowManager should need to know who "whoever
needs it" might be, Currently that is BrowserShortcutLauncherItemController
and Browser.

As I see it, our options are:
a) Use this workaround or something similar to allow SettingsWindowManager
to identify Settings browsers during chrome::Navigate.
b) Decorate Browser so that we do not need
SettingsWindowManager::IsSettingsWindow at all.

We were using chrome::IsTrustedPopupWindowWithScheme(browser,
content::kChromeUIScheme) to implement (b), but that is not reliable if the
Browser navigates away from a chrome:: page (and frankly was not a very
good way to do this in the first place).

I am not entirely against adding is_settings_window() to Browser, but I'm
not super enthusiastic about that either.

In the long (medium, maybe even short) run, I am hoping to make Settings a
WebUI and not a Browser at all, which will allow us to eliminate this hack
entirely.


On Fri, Sep 26, 2014 at 1:44 PM, Scott Violet <sky@chromium.org> wrote:

> What I dislike is having to maintain state assuming that
> IsSettingsBrowser is going to be invoked. This makes for awkward
> members in SettingsWindowManager. Can't that state be passed into
> whoever needs it?
>
> On Fri, Sep 26, 2014 at 9:39 AM,  <stevenjb@chromium.org> wrote:
> > On 2014/09/25 23:36:52, sky wrote:
> >
> >
>
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
> >>
> >> File chrome/browser/ui/settings_window_manager.cc (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
> >>
> >> chrome/browser/ui/settings_window_manager.cc:84: if (navigate_params_ &&
> >> navigate_params_->browser == browser)
> >> Dependencies like this are fragile and result in NavigateParams needing
> to
> >
> > talk
> >>
> >> to more things. I would rather you add something to NavigateParams that
> >> explicitly tells it the browser is a settings browser.
> >
> >
> > I'm not sure that I follow your suggestion. We rely on
> > NavigateParams->browser
> > for the Navigate() result, so it does not feel like we are adding any
> > dependencies here to the SettingsWindow code. Tacking on an extra
> parameter
> > to
> > NavigateParams just for this seems like adding an unnecessary dependency
> to
> > NavigateParams. Or are you suggesting that the Browser should know that
> it
> > is a
> > settings browser and we should use something like
> > browser->IsSettingsBrowser()
> > instead? I was trying to avoid tacking on yet another Browser property.
> >
> >
> >
> > https://codereview.chromium.org/595213005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-09-26 22:30:46]

I'm not too fond of adding something to browser, but this sure does
feel like a BrowserType. Can we pass something through the create
params?

On Fri, Sep 26, 2014 at 1:15 PM, Steven Bennetts <stevenjb@chromium.org> wrote:
> I don't think that SettingsWindowManager should need to know who "whoever
> needs it" might be, Currently that is BrowserShortcutLauncherItemController
> and Browser.
>
> As I see it, our options are:
> a) Use this workaround or something similar to allow SettingsWindowManager
> to identify Settings browsers during chrome::Navigate.
> b) Decorate Browser so that we do not need
> SettingsWindowManager::IsSettingsWindow at all.
>
> We were using chrome::IsTrustedPopupWindowWithScheme(browser,
> content::kChromeUIScheme) to implement (b), but that is not reliable if the
> Browser navigates away from a chrome:: page (and frankly was not a very good
> way to do this in the first place).
>
> I am not entirely against adding is_settings_window() to Browser, but I'm
> not super enthusiastic about that either.
>
> In the long (medium, maybe even short) run, I am hoping to make Settings a
> WebUI and not a Browser at all, which will allow us to eliminate this hack
> entirely.
>
>
> On Fri, Sep 26, 2014 at 1:44 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> What I dislike is having to maintain state assuming that
>> IsSettingsBrowser is going to be invoked. This makes for awkward
>> members in SettingsWindowManager. Can't that state be passed into
>> whoever needs it?
>>
>> On Fri, Sep 26, 2014 at 9:39 AM,  <stevenjb@chromium.org> wrote:
>> > On 2014/09/25 23:36:52, sky wrote:
>> >
>> >
>> >
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
>> >>
>> >> File chrome/browser/ui/settings_window_manager.cc (right):
>> >
>> >
>> >
>> >
>> >
https://codereview.chromium.org/595213005/diff/40001/chrome/browser/ui/settin...
>> >>
>> >> chrome/browser/ui/settings_window_manager.cc:84: if (navigate_params_
>> >> &&
>> >> navigate_params_->browser == browser)
>> >> Dependencies like this are fragile and result in NavigateParams needing
>> >> to
>> >
>> > talk
>> >>
>> >> to more things. I would rather you add something to NavigateParams that
>> >> explicitly tells it the browser is a settings browser.
>> >
>> >
>> > I'm not sure that I follow your suggestion. We rely on
>> > NavigateParams->browser
>> > for the Navigate() result, so it does not feel like we are adding any
>> > dependencies here to the SettingsWindow code. Tacking on an extra
>> > parameter
>> > to
>> > NavigateParams just for this seems like adding an unnecessary dependency
>> > to
>> > NavigateParams. Or are you suggesting that the Browser should know that
>> > it
>> > is a
>> > settings browser and we should use something like
>> > browser->IsSettingsBrowser()
>> > instead? I was trying to avoid tacking on yet another Browser property.
>> >
>> >
>> >
>> > https://codereview.chromium.org/595213005/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.