Add a testing switch to override the query for other forms of browsing history

Add a testing switch to override the query for other forms of browsing history

The API to query other forms of browsing history is not ready yet. The query
done by web history service therefore currently always return false, i.e.
"no other forms of browsing history found in this account".

Until the API is ready, provide a switch for testing purposes that will
change this to always return true.

BUG=602629
Committed: https://crrev.com/2b3e76a0b6974e86953e3786fad5ddafa59cd573
Cr-Commit-Position: refs/heads/master@{#388753}

[msramek, 2016-04-12 13:57:33]

msramek@chromium.org changed reviewers:
+ dbeam@chromium.org, jyquinn@chromium.org, msarda@chromium.org

[msramek, 2016-04-12 13:57:34]

Hi folks,

please have a look!

Thanks,
Martin

[msarda, 2016-04-12 16:41:48]

lgtm

https://codereview.chromium.org/1884553002/diff/40001/components/browsing_dat...
File components/browsing_data_ui/history_notice_utils.h (right):

https://codereview.chromium.org/1884553002/diff/40001/components/browsing_dat...
components/browsing_data_ui/history_notice_utils.h:20: extern const char
kOverrideOtherFormsOfBrowsingHistoryQuery[];
Please add a comment for this public constant.

[msramek, 2016-04-13 12:12:22]

Patchset #6 (id:100001) has been deleted

[msramek, 2016-04-13 12:12:42]

Patchset #5 (id:80001) has been deleted

[msramek, 2016-04-13 13:34:04]

Patchset #7 (id:150001) has been deleted

[msramek, 2016-04-13 13:55:53]

Patchset #7 (id:170001) has been deleted

[msramek, 2016-04-13 14:02:39]

Patchset #9 (id:230001) has been deleted

[msramek, 2016-04-13 14:26:00]

Patchset #10 (id:270001) has been deleted

[msramek, 2016-04-13 14:29:42]

Updates:
- Added iOS code.
- Now requiring exact URL for the switch to be enabled.
- Renamed history_ui.cc to history_ui.mm in order to be able to include
WebState, which is an Objective-C class.

PTAL!

https://codereview.chromium.org/1884553002/diff/40001/components/browsing_dat...
File components/browsing_data_ui/history_notice_utils.h (right):

https://codereview.chromium.org/1884553002/diff/40001/components/browsing_dat...
components/browsing_data_ui/history_notice_utils.h:20: extern const char
kOverrideOtherFormsOfBrowsingHistoryQuery[];
On 2016/04/12 16:41:47, msarda wrote:
> Please add a comment for this public constant.

Done.

[Dan Beam, 2016-04-14 03:31:14]

why do you want to check this in?  to make your local development easier?

that seems like a bad idea to send this to all users

[msramek, 2016-04-14 09:16:14]

On 2016/04/14 03:31:14, Dan Beam wrote:
> why do you want to check this in?  to make your local development easier?
> 
> that seems like a bad idea to send this to all users

No, for my local development I can simply remove the condition and recompile.
This is a switch for testers.

[Dan Beam, 2016-04-19 23:00:46]

https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
chrome/browser/ui/webui/history_ui.cc:198: void HistoryUI::DidStopLoading() {
why do we need to do this on DidStopLoading?  isn't the URL known earlier than
this?  it's what actually constructs this class...

https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
chrome/browser/ui/webui/history_ui.cc:214:
browsing_data_ui::switches::kOverrideOtherFormsOfBrowsingHistoryQuery);
why are you modifying the command line like this?

https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
chrome/browser/ui/webui/history_ui.cc:215:
web_ui()->GetWebContents()->GetController().LoadURL(
why do we need to change the URL?

https://codereview.chromium.org/1884553002/diff/310001/components/browsing_da...
File components/browsing_data_ui/history_notice_utils.cc (right):

https://codereview.chromium.org/1884553002/diff/310001/components/browsing_da...
components/browsing_data_ui/history_notice_utils.cc:26:
base::CommandLine::ForCurrentProcess()->HasSwitch(
why are you using a switch for this rather than like a static bool or something?

[msramek, 2016-04-20 15:10:38]

Patchset #11 (id:310001) has been deleted

[msramek, 2016-04-20 15:15:27]

Patchset #12 (id:350001) has been deleted

[msramek, 2016-04-20 15:22:45]

Patchset #11 (id:330001) has been deleted

[msramek, 2016-04-20 15:23:22]

Thanks, Dan! The code is much simpler now :)

Unfortunately, while I was cleaning up the intermediate patches that I used to
run iOS trybots, I accidentally also deleted the one with your comments. :-(
Sorry... let me respond inline.

On 2016/04/19 23:00:46, Dan Beam wrote:
>
https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/history_ui.cc (right):
> 
>
https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/history_ui.cc:198: void HistoryUI::DidStopLoading() {
> why do we need to do this on DidStopLoading?  isn't the URL known earlier than
> this?  it's what actually constructs this class...
> 

Done. It's really just that I followed a similar solution in
chrome://password-manager-internals. We can move it to the constructor, which
actually makes things simpler. And yes, now we don't even need to check the
exact URL.

>
https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/history_ui.cc:214:
> browsing_data_ui::switches::kOverrideOtherFormsOfBrowsingHistoryQuery);
> why are you modifying the command line like this?
> 

As mentioned below, I changed this to a static bool.

>
https://codereview.chromium.org/1884553002/diff/310001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/history_ui.cc:215:
> web_ui()->GetWebContents()->GetController().LoadURL(
> why do we need to change the URL?
> 

There were two reasons:
- Since the switch was turned on in DidStopLoading, the history page had to be
reloaded for it to take effect, as at that time it has already been rendered
with the assumption that the switch is off.
- I wanted to move from chrome://history/?reset_ofbh to chrome://history, so
that reloading the page would not reset the flag again

The former is already addressed by moving the code to the constructor. The
latter is perhaps not worth the additional complexity, so I removed it.

>
https://codereview.chromium.org/1884553002/diff/310001/components/browsing_da...
> File components/browsing_data_ui/history_notice_utils.cc (right):
> 
>
https://codereview.chromium.org/1884553002/diff/310001/components/browsing_da...
> components/browsing_data_ui/history_notice_utils.cc:26:
> base::CommandLine::ForCurrentProcess()->HasSwitch(
> why are you using a switch for this rather than like a static bool or
something?

Done. Right, static bool is enough in this case.

[msramek, 2016-04-20 15:24:23]

Jackie, as we discussed yesterday, there was a problem with the redirect on iOS.

By addressing Dan's comments, I removed that redirect. Will that work correctly
now?

[msramek, 2016-04-20 17:36:30]

I tested it with Android and it seems to work.

However, I realized that (at least on Android), one must open a new tab and type
"chrome://history/?reset_ofbh". The shortcut to open "History" from the menu and
then just add "?reset_ofbh" does not work, because HistoryUI outlives reloading
the history page, so the constructor doesn't run again. And that is why the
testing switch in chrome://password-manager-internals used DidStopLoading :)

But I'm still in favor of keeping the code simpler, so we don't have to fix
this.

[Jackie Quinn, 2016-04-20 23:28:39]

On 2016/04/20 17:36:30, msramek wrote:
> I tested it with Android and it seems to work.
> 
> However, I realized that (at least on Android), one must open a new tab and
type
> "chrome://history/?reset_ofbh". The shortcut to open "History" from the menu
and
> then just add "?reset_ofbh" does not work, because HistoryUI outlives
reloading
> the history page, so the constructor doesn't run again. And that is why the
> testing switch in chrome://password-manager-internals used DidStopLoading :)
> 
> But I'm still in favor of keeping the code simpler, so we don't have to fix
> this.

Works on iOS!

[Jackie Quinn, 2016-04-21 00:37:58]

On 2016/04/20 23:28:39, Jackie Quinn wrote:
> On 2016/04/20 17:36:30, msramek wrote:
> > I tested it with Android and it seems to work.
> > 
> > However, I realized that (at least on Android), one must open a new tab and
> type
> > "chrome://history/?reset_ofbh". The shortcut to open "History" from the menu
> and
> > then just add "?reset_ofbh" does not work, because HistoryUI outlives
> reloading
> > the history page, so the constructor doesn't run again. And that is why the
> > testing switch in chrome://password-manager-internals used DidStopLoading :)
> > 
> > But I'm still in favor of keeping the code simpler, so we don't have to fix
> > this.
> 
> Works on iOS!

lgtm

[Dan Beam, 2016-04-21 03:12:56]

lgtm

https://codereview.chromium.org/1884553002/diff/370001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/1884553002/diff/370001/chrome/browser/ui/webu...
chrome/browser/ui/webui/history_ui.cc:190:
browsing_data_ui::testing::kOverrideOtherFormsOfBrowsingHistoryQuery = true;
this is a constant naming form (kConstantThing)

this should probably be g_other_forms_of_browsing_history (if it's global) or
just other_forms_of_browsing_history otherwise

[msramek, 2016-04-21 08:50:30]

\o/

Thanks a lot for your help :-)

https://codereview.chromium.org/1884553002/diff/370001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/1884553002/diff/370001/chrome/browser/ui/webu...
chrome/browser/ui/webui/history_ui.cc:190:
browsing_data_ui::testing::kOverrideOtherFormsOfBrowsingHistoryQuery = true;
On 2016/04/21 03:12:55, Dan Beam wrote:
> this is a constant naming form (kConstantThing)
> 
> this should probably be g_other_forms_of_browsing_history (if it's global) or
> just other_forms_of_browsing_history otherwise

Done.

[msramek, 2016-04-21 08:51:32]

msramek@chromium.org changed reviewers:
+ droger@chromium.org

[msramek, 2016-04-21 08:51:33]

+David, can you please have a quick look at .gyp and .gn?

Thanks,
Martin

[droger, 2016-04-21 09:24:27]

gyp/gn lgtm

[msramek, 2016-04-21 09:36:05]

Thank you!

[msramek, 2016-04-21 09:36:09]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-04-21 09:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org,
dbeam@chromium.org, jyquinn@chromium.org
Link to the patchset: https://codereview.chromium.org/1884553002/#ps390001
(title: "Renamed the switch.")

[commit-bot: I haz the power, 2016-04-21 09:36:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884553002/390001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884553002/390001

[commit-bot: I haz the power, 2016-04-21 09:40:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 09:40:38]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-04-21 11:01:00]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2016-04-21 11:01:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884553002/390001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884553002/390001

[commit-bot: I haz the power, 2016-04-21 12:57:27]

Committed patchset #12 (id:390001)

[commit-bot: I haz the power, 2016-04-22 19:34:00]

Description was changed from

==========
Add a testing switch to override the query for other forms of browsing history

The API to query other forms of browsing history is not ready yet. The query
done by web history service therefore currently always return false, i.e.
"no other forms of browsing history found in this account".

Until the API is ready, provide a switch for testing purposes that will
change this to always return true.

BUG=602629
==========

to

==========
Add a testing switch to override the query for other forms of browsing history

The API to query other forms of browsing history is not ready yet. The query
done by web history service therefore currently always return false, i.e.
"no other forms of browsing history found in this account".

Until the API is ready, provide a switch for testing purposes that will
change this to always return true.

BUG=602629

Committed: https://crrev.com/2b3e76a0b6974e86953e3786fad5ddafa59cd573
Cr-Commit-Position: refs/heads/master@{#388753}
==========

[commit-bot: I haz the power, 2016-04-22 19:34:01]

Patchset 12 (id:??) landed as
https://crrev.com/2b3e76a0b6974e86953e3786fad5ddafa59cd573
Cr-Commit-Position: refs/heads/master@{#388753}