Show read-only messages for non-interact non-desktop Automation extensions

Show read-only messages for non-interact non-desktop Automation extensions

BUG=309681
Committed: https://crrev.com/429d7adac2adba3c254895787949e6574c87b742
Cr-Commit-Position: refs/heads/master@{#303185}

[aboxhall, 2014-07-22 23:58:38]

Following on from the discussion in the doc, here is the actual change for the
non-interact messages.

[not at google - send to devlin, 2014-07-23 01:56:59]

you might want to hold off on this until that other CL has landed.

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:41: bool read_only) {
bad news: this is all going to dramatically change in a moment:
https://codereview.chromium.org/404823009/

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.h (right):

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
extensions/common/permissions/permission_message_util.h:23: bool read_only =
false);
no default parameters, add another method instead.

[aboxhall, 2014-07-24 17:32:39]

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:41: bool read_only) {
On 2014/07/23 01:56:59, kalman wrote:
> bad news: this is all going to dramatically change in a moment:
> https://codereview.chromium.org/404823009/

Acknowledged.

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.h (right):

https://codereview.chromium.org/410023002/diff/40001/extensions/common/permis...
extensions/common/permissions/permission_message_util.h:23: bool read_only =
false);
On 2014/07/23 01:56:59, kalman wrote:
> no default parameters, add another method instead.

Done.

[aboxhall, 2014-07-25 18:06:19]

Ping? This is now based on the other CL.

[meacer, 2014-07-25 18:13:10]

Sorry for the late response. Lgtm.

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:69: int host_msg_id =
hosts.size() < kNumMessages
nit: This style seems to be more common:

bool ?
true :
false;

[not at google - send to devlin, 2014-07-25 18:17:43]

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:47: static const int
kNumMessages = 4;
we went to great lengths to avoid hard-coding a number here.

could you:
- remove it,
- do COMPILE_ASSERT(arraysize(kReadWriteMessagesList) ==
arraysize(kReadOnlyMessagesList)
- (the part I'm unsure about) make messages_list a pointer not an array
- either continue to use arraysize(kMessagesList) or assign it to a value like
messages_list_size.

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.h (right):

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.h:24: bool read_only);
ideally this would be an enum not a bool

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.h:26: const
std::set<std::string>& hosts);
could you remove this override? this only has 2 callers, one of which is
automation, so really you only need to update 1 caller.

[aboxhall, 2014-07-25 19:06:32]

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:47: static const int
kNumMessages = 4;
On 2014/07/25 18:17:42, kalman wrote:
> we went to great lengths to avoid hard-coding a number here.
> 
> could you:
> - remove it,

Done.

> - do COMPILE_ASSERT(arraysize(kReadWriteMessagesList) ==
> arraysize(kReadOnlyMessagesList)

Done.

> - (the part I'm unsure about) make messages_list a pointer not an array

It's not an array, it's a reference.

> - either continue to use arraysize(kMessagesList) or assign it to a value like
> messages_list_size.

Done (but it's assigned to a static const value).

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:69: int host_msg_id =
hosts.size() < kNumMessages
On 2014/07/25 18:13:10, Mustafa Emre Acer wrote:
> nit: This style seems to be more common:
> 
> bool ?
> true :
> false;
>  

Funny: this is how clang-format formats it. I'm going to leave as-is for that
reason.

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.h (right):

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.h:24: bool read_only);
On 2014/07/25 18:17:42, kalman wrote:
> ideally this would be an enum not a bool

Done.

https://codereview.chromium.org/410023002/diff/80001/extensions/common/permis...
extensions/common/permissions/permission_message_util.h:26: const
std::set<std::string>& hosts);
On 2014/07/25 18:17:42, kalman wrote:
> could you remove this override? this only has 2 callers, one of which is
> automation, so really you only need to update 1 caller.

Done.

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
extensions/common/permissions/permission_message_util.cc:67: const
MsgPair(&messages_list)[kNumMessages] =
Before you ask, this is how clang-format formatted it.

[not at google - send to devlin, 2014-07-25 20:40:04]

lgtm with 3 nits.

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
extensions/common/permissions/permission_message_util.cc:68: properties &
kReadOnly ? kReadOnlyMessagesList : kReadWriteMessagesList;
shouldn't this be "== kReadOnly"?

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
File extensions/common/permissions/permission_message_util.h (right):

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
extensions/common/permissions/permission_message_util.h:20: kReadOnly  = 0,
is the = 0, =1 really necessary?

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
extensions/common/permissions/permission_message_util.h:34: bool
exclude_file_scheme);
was this clang-format? it shouldn't be indented.

[aboxhall, 2014-07-25 20:54:22]

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
extensions/common/permissions/permission_message_util.cc:68: properties &
kReadOnly ? kReadOnlyMessagesList : kReadWriteMessagesList;
On 2014/07/25 20:40:03, kalman wrote:
> shouldn't this be "== kReadOnly"?

Ugh yeah, I think I was imagining it was a bitmask.

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
File extensions/common/permissions/permission_message_util.h (right):

https://codereview.chromium.org/410023002/diff/100001/extensions/common/permi...
extensions/common/permissions/permission_message_util.h:20: kReadOnly  = 0,
On 2014/07/25 20:40:03, kalman wrote:
> is the = 0, =1 really necessary?

Done.

[aboxhall, 2014-07-26 00:53:46]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-07-26 05:00:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/410023002/160001

[commit-bot: I haz the power, 2014-07-26 20:06:10]

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

[commit-bot: I haz the power, 2014-07-26 20:06:10]

Failed to apply patch for extensions/extensions_strings.grd:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/extensions_strings.grd
  Hunk #3 FAILED at 275.
  1 out of 3 hunks FAILED -- saving rejects to file
extensions/extensions_strings.grd.rej

Patch:       extensions/extensions_strings.grd
Index: extensions/extensions_strings.grd
diff --git a/extensions/extensions_strings.grd
b/extensions/extensions_strings.grd
index
ce0563709a9aeb5416b867e99cc61de368769084..d3fbdd7ec3517b40103ab05ba2e40f033148c2ee
100644
--- a/extensions/extensions_strings.grd
+++ b/extensions/extensions_strings.grd
@@ -196,17 +196,26 @@
       <message name="IDS_EXTENSION_MANIFEST_INVALID" desc="">
         Manifest file is invalid.
       </message>
-      
+
       <!-- Host access permissions. Please keep alphabetized. -->
       <message name="IDS_EXTENSION_PROMPT_WARNING_1_HOST" desc="Permission
string for access to data on one website.">
         Read and change your data on <ph
name="WEBSITE_1">$1<ex>www.google.com</ex></ph>
       </message>
+      <message name="IDS_EXTENSION_PROMPT_WARNING_1_HOST_READ_ONLY"
desc="Permission string for read-only access to data on one website.">
+        Read your data on <ph name="WEBSITE_1">$1<ex>www.google.com</ex></ph>
+      </message>
       <message name="IDS_EXTENSION_PROMPT_WARNING_2_HOSTS" desc="Permission
string for access to data on two websites.">
         Read and change your data on <ph
name="WEBSITE_1">$1<ex>www.google.com</ex></ph> and <ph
name="WEBSITE_2">$2<ex>www.reddit.com</ex></ph>
       </message>
+      <message name="IDS_EXTENSION_PROMPT_WARNING_2_HOSTS_READ_ONLY"
desc="Permission string for read-only access to data on two websites.">
+        Read your data on <ph name="WEBSITE_1">$1<ex>www.google.com</ex></ph>
and <ph name="WEBSITE_2">$2<ex>www.reddit.com</ex></ph>
+      </message>
       <message name="IDS_EXTENSION_PROMPT_WARNING_3_HOSTS" desc="Permission
string for access to data on three websites.">
         Read and change your data on <ph
name="WEBSITE_1">$1<ex>www.google.com</ex></ph>, <ph
name="WEBSITE_2">$2<ex>www.reddit.com</ex></ph>, and <ph
name="WEBSITE_3">$3<ex>news.ycombinator.com</ex></ph>
       </message>
+      <message name="IDS_EXTENSION_PROMPT_WARNING_3_HOSTS_READ_ONLY"
desc="Permission string for read-only access to data on three websites.">
+        Read your data on <ph name="WEBSITE_1">$1<ex>www.google.com</ex></ph>,
<ph name="WEBSITE_2">$2<ex>www.reddit.com</ex></ph>, and <ph
name="WEBSITE_3">$3<ex>news.ycombinator.com</ex></ph>
+      </message>
       <message name="IDS_EXTENSION_PROMPT_WARNING_HOST_AND_SUBDOMAIN"
desc="Permission string requesting access to data on a website and its
sub-domains.">
         all <ph name="WEBSITE_1">$1<ex>google.com</ex></ph> sites
       </message>
@@ -216,6 +225,9 @@
       <message name="IDS_EXTENSION_PROMPT_WARNING_HOSTS_LIST" desc="Permission
string for access to data for an arbitrary list of websites.">
         Read and change your data on a number of websites
       </message>
+      <message name="IDS_EXTENSION_PROMPT_WARNING_HOSTS_LIST_READ_ONLY"
desc="Permission string for read-only access to data for an arbitrary list of
websites.">
+        Read your data on a number of websites
+      </message>
       <message name="IDS_EXTENSION_PROMPT_WARNING_HOST_LIST_ENTRY" desc="Single
entry in a list of websites.">
         - <ph name="WEBSITE_1">$1<ex>www.google.com</ex></ph>
       </message>
@@ -263,7 +275,7 @@
       <message name="IDS_EXTENSION_PROMPT_WARNING_SOCKET_SPECIFIC_HOSTS"
desc="Permission string for access to multiple specific computers on the local
network or internet.">
         Exchange data with the computers named: <ph
name="HOSTNAMES">$1<ex>foo.example.com bar.example.com</ex></ph>
       </message>
-      
+
       <!-- USB API strings. Please keep alphabetized. -->
       <message name="IDS_EXTENSION_PROMPT_WARNING_USB" desc="Permission string
for access to USB devices.">
         Access your USB devices

[aboxhall, 2014-11-07 03:28:26]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-11-07 03:29:36]

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

[commit-bot: I haz the power, 2014-11-07 04:25:24]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-11-07 04:26:00]

Patchset 11 (id:??) landed as
https://crrev.com/429d7adac2adba3c254895787949e6574c87b742
Cr-Commit-Position: refs/heads/master@{#303185}