Replaced the current WebsiteSettings dialog with a new PageInfo dialog

Replaced the current WebsiteSettings dialog with a new PageInfo dialog

Started replacing the current WebsiteSettings dialog with a simple
implementation of the upcoming PageInfo dialog. Currently, the dialog
only contains the URL, a short message about the site and a 'Done'
button to close the dialog. Later, it will also contain individual
dropdowns to configure permissions for the website's content settings.

The new dialog is hidden behind the flag
--enable-new-website-settings. Without the flag, the old dialog will
be displayed.

BUG=365528
Committed: https://crrev.com/fa1f0b371e82dac98cdbd4e14cce09777bd0c660
Cr-Commit-Position: refs/heads/master@{#299039}

[sashab, 2014-08-28 00:21:23]

sashab@chromium.org changed reviewers:
+ newt@chromium.org, tedchoc@chromium.org

[sashab, 2014-08-28 00:21:23]

I was asked to replace the existing WebsiteSettings dialog with this PageInfo
one - the information on the old dialog may be added back at a later stage,
probably in a simpler, easier-to-understand format for users. This is the first
of probably 2-3 patches that will build up the contents of this dialog. I've
tried to keep them small and simple so they are easier to review.

This is the first Android patch I have attempted (I just finished the Android
bootcamp), so please give feedback where I may have not followed the
style/practices of Clank. I have tried where possible to copy existing places,
so hopefully it's at least close :)

Thanks,

Sasha

[sashab, 2014-08-28 07:08:37]

Ok, I've now moved WebsiteSettingsPopup.java (and its corresponding cc/h/xml
files) to WebsiteSettingsPopupLegacy, and moved all the new content to
WebsiteSettingsPopup. It's hidden behind a flag 'enable-new-website-settings',
and the static function WebsiteSettingsPopup.show() chooses which one to display
(so its invisible to clank).

You don't have to look at the changes to WebsiteSettingsPopupLegacy - I've just
renamed where needed, and fixed up some small formatting fixes where the
presubmit check complained (I could have reformatted the whole file, but there's
no point). Any other changes were just from "git cl format".

Basically the review is almost exactly the same as before - there's now just a
few more files, but they are just existing code that's been moved around from
the rename :) Hopefully its still clear and you can still tell what needs to be
reviewed. I'm going to update the CL comment as well and there are screenshots
on the bug.

Thanks,

Sasha

[sashab, 2014-09-01 23:09:15]

Please review. I'm going off the mocks from:
www.crbug.com/394097

Currently, the functionality is hidden behind a flag, but:
- The mocks match for the requested types of URLs (strings in
https://docs.google.com/a/google.com/document/d/1Db6KamnUbNmR0_h0QuAtlEvdM1N7...)

Known issues (will be fixed in follow-ups):
- Https does not have a strikethrough (can't see an easy way to do this without
having two separate textviews)
- The start of the message text for Broken HTTPS is not red and bolded
- Phishing/malware sites not detected

Thanks,

Sasha

[Ted C, 2014-09-03 20:44:49]

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:40:
final long mNativeWebsiteSettingsPopup;
can this be private?

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:46:
private final TextView mUrlTitle, mUrlConnectionMessage;
the previous file set a bad precedent, but I would put each on their own
separate line.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:82:
@Override
this should be -4 indented

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:85:
// the data we're
why reformat this comment?  it looks like it all fit in 100 chars before and now
it's oddly formatted now.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:91:
@Override
-4 indent here too

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:103:
* |connection_type| should be a valid PageInfoConnectionType.
the |variable| syntax is more for C++, it's fine here as these are private
methods but I wouldn't use it for public stuff.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:107:
boolean make_domain_bold = false;
s/make_domain_bold/makeDomainBold

same camel casing comment for the rest of these variables.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:109:
if (connection_type == PageInfoConnectionType.CONNECTION_UNKNOWN) {
I would probably use a switch here for connectionType instead of the if/else ifs

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:125:
}
I would have a else/default case where you:

assert false : "Unexpected connection type: " + connectionType;

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:135:
final ForegroundColorSpan domain_color_span = new ForegroundColorSpan(
I would probably drop the finals in this method as they aren't super consistent

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:163:
scrollView.setLayoutParams(new LinearLayout.LayoutParams(
you shouldn't need to set the layout params here as you're respecifying them
down below in the addContentView calls.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:174:
window.setLayout(ViewGroup.LayoutParams.MATCH_PARENT,
what are the plans for tablet?  I'm doubtful we want this to go full width on a
10" screen

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:187:
*            This information is retrieved for the visible entry.
align this with "The WebContents" above.

https://codereview.chromium.org/516703002/diff/100001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_android.h (right):

https://codereview.chromium.org/516703002/diff/100001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_android.h:21: #include
"ui/gfx/android/page_info_connection_type_list.h"
I think the new method of generating enums is probably going to land soon, so we
might need to switch over depending on which gets there first.

[sashab, 2014-09-19 02:29:48]

Alright, all working! PTAL :)

Now I just need some help with the tablet stuff... Should this look different on
a tablet screen? No mocks for it so maybe we should just do whatever the
'default' is, not sure how much I should hardcode and how much automatically
upscales.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:40:
final long mNativeWebsiteSettingsPopup;
On 2014/09/03 20:44:48, Ted C wrote:
> can this be private?

Done.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:46:
private final TextView mUrlTitle, mUrlConnectionMessage;
On 2014/09/03 20:44:48, Ted C wrote:
> the previous file set a bad precedent, but I would put each on their own
> separate line.

Done.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:82:
@Override
On 2014/09/03 20:44:48, Ted C wrote:
> this should be -4 indented

I think the provided formatter I was using in Eclipse was misconfigured. Sorry
about that :)

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:85:
// the data we're
On 2014/09/03 20:44:48, Ted C wrote:
> why reformat this comment?  it looks like it all fit in 100 chars before and
now
> it's oddly formatted now.

Sorry, auto-format by Eclipse. Fixed.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:91:
@Override
On 2014/09/03 20:44:48, Ted C wrote:
> -4 indent here too

Done.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:103:
* |connection_type| should be a valid PageInfoConnectionType.
On 2014/09/03 20:44:48, Ted C wrote:
> the |variable| syntax is more for C++, it's fine here as these are private
> methods but I wouldn't use it for public stuff.

Is there any way I should distinguish them? I'd rather have it all in the same
style. Just removed the |'s for now.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:107:
boolean make_domain_bold = false;
On 2014/09/03 20:44:48, Ted C wrote:
> s/make_domain_bold/makeDomainBold
> 
> same camel casing comment for the rest of these variables.

Done.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:109:
if (connection_type == PageInfoConnectionType.CONNECTION_UNKNOWN) {
On 2014/09/03 20:44:48, Ted C wrote:
> I would probably use a switch here for connectionType instead of the if/else
ifs

Good idea, done.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:125:
}
On 2014/09/03 20:44:48, Ted C wrote:
> I would have a else/default case where you:
> 
> assert false : "Unexpected connection type: " + connectionType;

Done.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:135:
final ForegroundColorSpan domain_color_span = new ForegroundColorSpan(
On 2014/09/03 20:44:48, Ted C wrote:
> I would probably drop the finals in this method as they aren't super
consistent

Done.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:163:
scrollView.setLayoutParams(new LinearLayout.LayoutParams(
On 2014/09/03 20:44:48, Ted C wrote:
> you shouldn't need to set the layout params here as you're respecifying them
> down below in the addContentView calls.

Aren't these the layout params for the scrollView's parent? It needs to stretch
to fill the surrounding container. Removed for now because I think I
misunderstand what these do, since it still seems to work.

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:174:
window.setLayout(ViewGroup.LayoutParams.MATCH_PARENT,
On 2014/09/03 20:44:48, Ted C wrote:
> what are the plans for tablet?  I'm doubtful we want this to go full width on
a
> 10" screen

I don't have mocks for a tablet version yet... What do we usually do? And how
can I make this only go full-width on a tablet?

https://codereview.chromium.org/516703002/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:187:
*            This information is retrieved for the visible entry.
On 2014/09/03 20:44:48, Ted C wrote:
> align this with "The WebContents" above.

Done.

https://codereview.chromium.org/516703002/diff/100001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_android.h (right):

https://codereview.chromium.org/516703002/diff/100001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_android.h:21: #include
"ui/gfx/android/page_info_connection_type_list.h"
On 2014/09/03 20:44:49, Ted C wrote:
> I think the new method of generating enums is probably going to land soon, so
we
> might need to switch over depending on which gets there first.

Ahh... And it's landed. Alright, I'll switch over this code :)

[Ted C, 2014-09-23 03:56:15]

tedchoc@chromium.org changed reviewers:
+ cjhopman@chromium.org

[Ted C, 2014-09-23 03:56:16]

lgtm w/ nits

+cjhopman because I think you'll need to update some .gn files as well.

https://codereview.chromium.org/516703002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/516703002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:76:
window.setBackgroundDrawable(new ColorDrawable(Color.TRANSPARENT));
Will null work instead of a color drawable with transparent?

https://codereview.chromium.org/516703002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:191:
*                     This information is retrieved for the visible entry.
This is indented one too many

https://codereview.chromium.org/516703002/diff/140001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/516703002/diff/140001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:344: resource_id =
IDS_PAGE_INFO_UNKNOWN_CONNECTION_SUMMARY_TEXT;
Instead of keeping a resource_id reference, I would just return in these cases.

[cjhopman, 2014-09-25 21:17:53]

`git cl try` will include some gn bots in the list it uses, or you can manually
try one like "android_chromium_gn_compile_rel"

You can also manually build using gn like this (you don't have to do this, the
trybots are probably enough):
gn gen out-gn/mobile '--args=os="android" cpu_arch="arm" use_goma=true'
ninja -C out-gn/mobile -j1000

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:684: 'target_name': 'page_info_connection_type_java',
This will need to be added to GN eventually, but chrome_java isn't building
there yet (and all the other java_cpp_template/enum stuff in this file isn't
there yet), so you shouldn't need to worry about that.

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:2753:
'android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopupLegacy.java',
This should automatically be included in GN as it shares this list.

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:68:
'browser/ui/android/website_settings_popup_legacy_android.h',
This should automatically be included in GN as it shares this list.

https://codereview.chromium.org/516703002/diff/160001/ui/android/ui_android.gyp
File ui/android/ui_android.gyp (right):

https://codereview.chromium.org/516703002/diff/160001/ui/android/ui_android.g...
ui/android/ui_android.gyp:31: 'target_name': 'page_info_connection_type_java',
This (and the dependency below) will have to be added to GN.

It looks like ui is using a single target for all java_cpp_enum and so you will
just need to add the sources + outputs in it. See ui/android/BUILD.gn and find
the java_enums_srcjar target.

[sashab, 2014-09-29 04:17:10]

sashab@chromium.org changed reviewers:
+ ben@chromium.org, benm@chromium.org

[sashab, 2014-09-29 04:17:12]

benm@chromium.org: Please review changes in android_webview/

ben@chromium.org: Please review changes in chrome/chrome.gyp and
chrome/browser/ui/*

https://codereview.chromium.org/516703002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/516703002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:76:
window.setBackgroundDrawable(new ColorDrawable(Color.TRANSPARENT));
On 2014/09/23 03:56:16, Ted C (OOO till 10.1) wrote:
> Will null work instead of a color drawable with transparent?

They are slightly different. 'null' leaves a gap around the dialog; a
transparent drawable removes this gap. I'm guessing it has something to do with
'null' not overriding the default setting (but an actual drawable does) but I'm
not sure. Should I comment this?

https://codereview.chromium.org/516703002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:191:
*                     This information is retrieved for the visible entry.
On 2014/09/23 03:56:16, Ted C wrote:
> This is indented one too many

Done.

https://codereview.chromium.org/516703002/diff/140001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/516703002/diff/140001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:344: resource_id =
IDS_PAGE_INFO_UNKNOWN_CONNECTION_SUMMARY_TEXT;
On 2014/09/23 03:56:16, Ted C wrote:
> Instead of keeping a resource_id reference, I would just return in these
cases.

No prob - was copying from above but they are actually slightly different :)
Done.

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:684: 'target_name': 'page_info_connection_type_java',
On 2014/09/25 21:17:52, cjhopman wrote:
> This will need to be added to GN eventually, but chrome_java isn't building
> there yet (and all the other java_cpp_template/enum stuff in this file isn't
> there yet), so you shouldn't need to worry about that.

In that case, this won't build with GN at all. The UI stuff below is a mistake.

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:2753:
'android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopupLegacy.java',
On 2014/09/25 21:17:52, cjhopman wrote:
> This should automatically be included in GN as it shares this list.

Great!

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/516703002/diff/160001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:68:
'browser/ui/android/website_settings_popup_legacy_android.h',
On 2014/09/25 21:17:52, cjhopman wrote:
> This should automatically be included in GN as it shares this list.

Also great!

https://codereview.chromium.org/516703002/diff/160001/ui/android/ui_android.gyp
File ui/android/ui_android.gyp (right):

https://codereview.chromium.org/516703002/diff/160001/ui/android/ui_android.g...
ui/android/ui_android.gyp:31: 'target_name': 'page_info_connection_type_java',
On 2014/09/25 21:17:52, cjhopman wrote:
> This (and the dependency below) will have to be added to GN.
> 
> It looks like ui is using a single target for all java_cpp_enum and so you
will
> just need to add the sources + outputs in it. See ui/android/BUILD.gn and find
> the java_enums_srcjar target.

This is actually a duplicate definition of the target as a result of a merge
conflict; this is not used, so I've removed it.

[palmer, 2014-09-30 21:44:29]

palmer@chromium.org changed reviewers:
+ palmer@chromium.org

[palmer, 2014-09-30 21:44:30]

https://codereview.chromium.org/516703002/diff/180001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/516703002/diff/180001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:617: Chromium could not detect your connection
to this site.
If I were a normal user, I would not know what this string means. How about:

"Chromium could not determine whether or not your connection to this site is
private."

https://codereview.chromium.org/516703002/diff/180001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/516703002/diff/180001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9345: +      <message
name="IDS_PAGE_INFO_ENCRYPTED_CONNECTION_SUMMARY_TEXT" desc="Message to display
in the page info bubble when the connection is encrypted.">
Nit: It might be better to use the same vocabular in the desc=... field as in
the actual string. I.e. s/encrypted/private/.

https://codereview.chromium.org/516703002/diff/180001/chrome/app/google_chrom...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/516703002/diff/180001/chrome/app/google_chrom...
chrome/app/google_chrome_strings.grd:541: Google Chrome could not detect your
connection to this site.
Same as for Chromium.

[sashab, 2014-10-01 01:31:14]

https://codereview.chromium.org/516703002/diff/180001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/516703002/diff/180001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:617: Chromium could not detect your connection
to this site.
On 2014/09/30 21:44:30, Chromium Palmer wrote:
> If I were a normal user, I would not know what this string means. How about:
> 
> "Chromium could not determine whether or not your connection to this site is
> private."

All the security strings have been discussed and approved by both UX and
security in this doc:
https://docs.google.com/a/google.com/document/d/1Db6KamnUbNmR0_h0QuAtlEvdM1N7...

PTAL and raise this issue there if you would like to change it :) I plan to land
this now with the approved strings (it's behind a flag for now anyway though).

https://codereview.chromium.org/516703002/diff/180001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/516703002/diff/180001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9345: +      <message
name="IDS_PAGE_INFO_ENCRYPTED_CONNECTION_SUMMARY_TEXT" desc="Message to display
in the page info bubble when the connection is encrypted.">
On 2014/09/30 21:44:30, Chromium Palmer wrote:
> Nit: It might be better to use the same vocabular in the desc=... field as in
> the actual string. I.e. s/encrypted/private/.

Yup, thanks, done.

[benm (inactive), 2014-10-01 15:33:53]

https://codereview.chromium.org/516703002/diff/220001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/516703002/diff/220001/android_webview/java_li...
android_webview/java_library_common.mk:45: $(call
intermediates-dir-for,GYP,shared)/enums/page_info_connection_type/org/chromium/chrome/browser/PageInfoConnectionType.java
\
I think that you also need to update android_webview.gyp, per the comment on
line 39.

[sashab, 2014-10-02 00:37:24]

https://codereview.chromium.org/516703002/diff/220001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/516703002/diff/220001/android_webview/java_li...
android_webview/java_library_common.mk:45: $(call
intermediates-dir-for,GYP,shared)/enums/page_info_connection_type/org/chromium/chrome/browser/PageInfoConnectionType.java
\
On 2014/10/01 15:33:52, benm wrote:
> I think that you also need to update android_webview.gyp, per the comment on
> line 39.

Not sure what this comment means, since android_webview.gyp doesn't contain any
of these files. Changed the list in libwebviewchromium.gypi (since I believe
this is what it's actually referring to) but left the comment for now. Going to
discuss with mkosiba@chromium.org who is the git blame for this line what it
actually means.

[benm (inactive), 2014-10-06 09:04:07]

https://codereview.chromium.org/516703002/diff/240001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/516703002/diff/240001/android_webview/java_li...
android_webview/java_library_common.mk:47: $(call
intermediates-dir-for,GYP,shared)/enums/page_info_connection_type/org/chromium/chrome/browser/PageInfoConnectionType.java
\
Hold up, sorry I didn't notice this before. WebView cannot depend on anything in
the chrome/ layer. It needs to move to content/, or a component.

It looks like all the other changes in this CL are in chrome/, so I'm not sure
why we need this diff at all here?

https://codereview.chromium.org/516703002/diff/240001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
(right):

https://codereview.chromium.org/516703002/diff/240001/content/public/android/...
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:25:
public static final String ENABLE_NEW_WEBSITE_SETTINGS =
"enable-new-website-settings";
as the new stuff is all in chrome/, why are we adding a switch to content/ ?

[sashab, 2014-10-06 23:11:55]

Added a ChromeSwitches file and removed PageInfoConnectionType  from
AndroidWebview.

https://codereview.chromium.org/516703002/diff/240001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/516703002/diff/240001/android_webview/java_li...
android_webview/java_library_common.mk:47: $(call
intermediates-dir-for,GYP,shared)/enums/page_info_connection_type/org/chromium/chrome/browser/PageInfoConnectionType.java
\
On 2014/10/06 09:04:07, benm wrote:
> Hold up, sorry I didn't notice this before. WebView cannot depend on anything
in
> the chrome/ layer. It needs to move to content/, or a component.
> 
> It looks like all the other changes in this CL are in chrome/, so I'm not sure
> why we need this diff at all here?

No, Android Webview doesn't need PageInfoConnectionType anyway. Removed the
changes to both this and libwebviewchromium.gypi.

https://codereview.chromium.org/516703002/diff/240001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
(right):

https://codereview.chromium.org/516703002/diff/240001/content/public/android/...
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:25:
public static final String ENABLE_NEW_WEBSITE_SETTINGS =
"enable-new-website-settings";
On 2014/10/06 09:04:07, benm wrote:
> as the new stuff is all in chrome/, why are we adding a switch to content/ ?

Sorry, I didn't realise there were different switch files for Android. Moved
this to a new chrome/ switches file.

[benm (inactive), 2014-10-07 09:51:05]

On 2014/10/06 23:11:55, sasha_b wrote:
> Added a ChromeSwitches file and removed PageInfoConnectionType  from
> AndroidWebview.
> 
>
https://codereview.chromium.org/516703002/diff/240001/android_webview/java_li...
> File android_webview/java_library_common.mk (right):
> 
>
https://codereview.chromium.org/516703002/diff/240001/android_webview/java_li...
> android_webview/java_library_common.mk:47: $(call
>
intermediates-dir-for,GYP,shared)/enums/page_info_connection_type/org/chromium/chrome/browser/PageInfoConnectionType.java
> \
> On 2014/10/06 09:04:07, benm wrote:
> > Hold up, sorry I didn't notice this before. WebView cannot depend on
anything
> in
> > the chrome/ layer. It needs to move to content/, or a component.
> > 
> > It looks like all the other changes in this CL are in chrome/, so I'm not
sure
> > why we need this diff at all here?
> 
> No, Android Webview doesn't need PageInfoConnectionType anyway. Removed the
> changes to both this and libwebviewchromium.gypi.
> 
>
https://codereview.chromium.org/516703002/diff/240001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
> (right):
> 
>
https://codereview.chromium.org/516703002/diff/240001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:25:
> public static final String ENABLE_NEW_WEBSITE_SETTINGS =
> "enable-new-website-settings";
> On 2014/10/06 09:04:07, benm wrote:
> > as the new stuff is all in chrome/, why are we adding a switch to content/ ?
> 
> Sorry, I didn't realise there were different switch files for Android. Moved
> this to a new chrome/ switches file.

Thanks Sasha, in the context of android_webview this looks great now ;-)

Ted may want to take another pass.

[Ted C, 2014-10-07 17:48:06]

still lgtm w/ some small final nits

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/res...
File chrome/android/java/res/layout/website_settings_legacy.xml (right):

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/res...
chrome/android/java/res/layout/website_settings_legacy.xml:19:
android:layout_width="wrap_content"
why switch this around?  in general the layout params should be next to each
other

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/ChromeSwitches.java (right):

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/ChromeSwitches.java:1: // Copyright
2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/516703002/diff/300001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_legacy_android.h (right):

https://codereview.chromium.org/516703002/diff/300001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_legacy_android.h:33: virtual
void SetCookieInfo(const CookieInfoList& cookie_info_list) OVERRIDE;
You'll want to update to using lowercase override as that is the new standard.

[sashab, 2014-10-07 22:58:22]

sashab@chromium.org changed reviewers:
+ gbillock@chromium.org, thestig@chromium.org
- ben@chromium.org

[sashab, 2014-10-07 22:58:24]

-ben

gbillock: ptal at chrome/browser/ui/website_settings :)
thestig: ptal at chrome/chrome.gyp :)

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/res...
File chrome/android/java/res/layout/website_settings_legacy.xml (right):

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/res...
chrome/android/java/res/layout/website_settings_legacy.xml:19:
android:layout_width="wrap_content"
On 2014/10/07 17:48:06, Ted C wrote:
> why switch this around?  in general the layout params should be next to each
> other

Oops, not sure how this happened, fixed :)

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/ChromeSwitches.java (right):

https://codereview.chromium.org/516703002/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/ChromeSwitches.java:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2014/10/07 17:48:06, Ted C wrote:
> 2014

Done.

https://codereview.chromium.org/516703002/diff/300001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_legacy_android.h (right):

https://codereview.chromium.org/516703002/diff/300001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_legacy_android.h:33: virtual
void SetCookieInfo(const CookieInfoList& cookie_info_list) OVERRIDE;
On 2014/10/07 17:48:06, Ted C wrote:
> You'll want to update to using lowercase override as that is the new standard.

Done.

[sashab, 2014-10-07 23:05:27]

sashab@chromium.org changed reviewers:
+ markusheintz@chromium.org
- gbillock@chromium.org

[Lei Zhang, 2014-10-07 23:30:31]

https://codereview.chromium.org/516703002/diff/340001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/516703002/diff/340001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9443: +      <message
name="IDS_PAGE_INFO_ENCRYPTED_CONNECTION_SUMMARY_TEXT" desc="Message to display
in the page info bubble when the connection is private.">
Are these in an <if expr="is_android"> section? If not, should they be?

https://codereview.chromium.org/516703002/diff/340001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/516703002/diff/340001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:7: #include
"grit/chromium_strings.h"
Please use chrome/grit.

https://codereview.chromium.org/516703002/diff/340001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/516703002/diff/340001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:633: 'target_name': 'page_info_connection_type_java',
Do you need to create the equivalent target in GN?

[sashab, 2014-10-08 00:04:21]

Thanks thestig - feedback done.

Re-assigned website_settings review to markusheintz after chat with gbillock :)

https://codereview.chromium.org/516703002/diff/340001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/516703002/diff/340001/chrome/app/generated_re...
chrome/app/generated_resources.grd:9443: +      <message
name="IDS_PAGE_INFO_ENCRYPTED_CONNECTION_SUMMARY_TEXT" desc="Message to display
in the page info bubble when the connection is private.">
On 2014/10/07 23:30:31, Lei Zhang wrote:
> Are these in an <if expr="is_android"> section? If not, should they be?

Yes, good thinking. Done.

https://codereview.chromium.org/516703002/diff/340001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/516703002/diff/340001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:7: #include
"grit/chromium_strings.h"
On 2014/10/07 23:30:31, Lei Zhang wrote:
> Please use chrome/grit.

Sure, I didn't know this was a thing. Updated all 3 of them.

https://codereview.chromium.org/516703002/diff/340001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/516703002/diff/340001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:633: 'target_name': 'page_info_connection_type_java',
On 2014/10/07 23:30:31, Lei Zhang wrote:
> Do you need to create the equivalent target in GN?

Nope, this has already all been discussed :) GN doesn't support the chrome_java
target yet.

[Lei Zhang, 2014-10-08 00:07:33]

lgtm

https://codereview.chromium.org/516703002/diff/360001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/516703002/diff/360001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:9: #include
"chrome/grit/theme_resources.h"
Sorry, this one should be just "grit/..." because it hasn't been converted yet.

[sashab, 2014-10-08 01:08:09]

https://codereview.chromium.org/516703002/diff/360001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/516703002/diff/360001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:9: #include
"chrome/grit/theme_resources.h"
On 2014/10/08 00:07:32, Lei Zhang wrote:
> Sorry, this one should be just "grit/..." because it hasn't been converted
yet.

Done.

[markusheintz_, 2014-10-08 17:06:12]

On 2014/10/08 01:08:09, sasha_b wrote:
>
https://codereview.chromium.org/516703002/diff/360001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings_ui.cc (right):
> 
>
https://codereview.chromium.org/516703002/diff/360001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings_ui.cc:9: #include
> "chrome/grit/theme_resources.h"
> On 2014/10/08 00:07:32, Lei Zhang wrote:
> > Sorry, this one should be just "grit/..." because it hasn't been converted
> yet.
> 
> Done.

Website Settings parts LGTM

[Lei Zhang, 2014-10-08 18:43:30]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-10-08 18:46:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/516703002/380001

[commit-bot: I haz the power, 2014-10-08 19:05:09]

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

[commit-bot: I haz the power, 2014-10-08 19:05:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2014-10-09 22:27:32]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-09 22:28:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/516703002/400001

[commit-bot: I haz the power, 2014-10-09 23:10:41]

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

[commit-bot: I haz the power, 2014-10-09 23:10:43]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sashab, 2014-10-10 01:15:17]

https://codereview.chromium.org/516703002/diff/530001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/516703002/diff/530001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:158: static int
GetConnectionSummaryMessageID(
Could have made this function android only (since it's currently only used on
android), but this will likely be needed when we mirror the current PageInfo to
desktop. So left it as non-android-only for now, and removed the android guard
from the messages file.

[sashab, 2014-10-10 01:15:28]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-10 01:17:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/516703002/530001

[commit-bot: I haz the power, 2014-10-10 02:03:56]

Committed patchset #22 (id:530001)

[commit-bot: I haz the power, 2014-10-10 02:05:14]

Patchset 22 (id:??) landed as
https://crrev.com/fa1f0b371e82dac98cdbd4e14cce09777bd0c660
Cr-Commit-Position: refs/heads/master@{#299039}