Add information that some data may not get deleted

Add information that some data may not get deleted

BUG=137412
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241079

[battre, 2013-12-09 15:01:11]

Hi Evan,

could you please review this CL?

I'll send you a screenshot by email.

Thanks,
Dominic

[Evan Stade, 2013-12-09 19:07:12]

https://codereview.chromium.org/108273002/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/108273002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7719: +        hosted app data
nit: capitalization

https://codereview.chromium.org/108273002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7746: +        Saved <ph
name="BEGIN_LINK">&lt;button id="open-content-settings-from-clear-browsing-data"
class="link-button"&gt;</ph>content settings<ph
name="END_LINK">&lt;/button&gt;</ph> and <ph name="BEGIN2_LINK">&lt;button
id="open-search-engines-from-clear-browsing-data"
class="link-button"&gt;</ph>search engines<ph
name="END2_LINK">&lt;/button&gt;</ph> will not be cleared and may reflect your
browsing habits.
this makes me feel a bit queasy; it's hard to read, it spreads important code
like IDs into a .grd file, we have to rely on translators not to mess it up or
functionality is lost, and the use of innerhtml creates a potential
vulnerability. Can you do something like
IDS_AUTOFILL_WALLET_BUYER_COUNTRY_NOT_SUPPORTED?

https://codereview.chromium.org/108273002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

https://codereview.chromium.org/108273002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/clear_browser_data_overlay.css:34: margin: 0
;

https://codereview.chromium.org/108273002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/clear_browser_data_overlay.css:37:
#open-content-settings-from-clear-browsing-data {
#some-stuff-remains-footer a {
  padding: 0;
}

[battre, 2013-12-11 06:10:29]

Hi Evan,

could you please take another look and let me know whether I understood your
recommendation correctly?

Thanks,
Dominic

https://codereview.chromium.org/108273002/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/108273002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7719: +        hosted app data
On 2013/12/09 19:07:13, Evan Stade wrote:
> nit: capitalization

Done.

https://codereview.chromium.org/108273002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7746: +        Saved <ph
name="BEGIN_LINK">&lt;button id="open-content-settings-from-clear-browsing-data"
class="link-button"&gt;</ph>content settings<ph
name="END_LINK">&lt;/button&gt;</ph> and <ph name="BEGIN2_LINK">&lt;button
id="open-search-engines-from-clear-browsing-data"
class="link-button"&gt;</ph>search engines<ph
name="END2_LINK">&lt;/button&gt;</ph> will not be cleared and may reflect your
browsing habits.
On 2013/12/09 19:07:13, Evan Stade wrote:
> this makes me feel a bit queasy; it's hard to read, it spreads important code
> like IDs into a .grd file, we have to rely on translators not to mess it up or
> functionality is lost, and the use of innerhtml creates a potential
> vulnerability. Can you do something like
> IDS_AUTOFILL_WALLET_BUYER_COUNTRY_NOT_SUPPORTED?

Done.

https://codereview.chromium.org/108273002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

https://codereview.chromium.org/108273002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/clear_browser_data_overlay.css:34: margin: 0
On 2013/12/09 19:07:13, Evan Stade wrote:
> ;

Done.

https://codereview.chromium.org/108273002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/clear_browser_data_overlay.css:37:
#open-content-settings-from-clear-browsing-data {
On 2013/12/09 19:07:13, Evan Stade wrote:
> #some-stuff-remains-footer a {
>   padding: 0;
> }

Done.

[battre, 2013-12-12 04:43:37]

Friendly ping (I'd like to land this before the branch point)

Best regards,
Dominic

[Evan Stade, 2013-12-13 17:55:04]

https://codereview.chromium.org/108273002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/clear_browser_data_overlay.js (right):

https://codereview.chromium.org/108273002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/clear_browser_data_overlay.js:65:
stuffRemainsFooter.innerHTML.replace(/\[(.*)\]/,
nit: can you get all the text before the first link, put it in a span, append
the span, get all the text inside the first link, put it in a button, append the
button, etc.

In general it's preferable to avoid innerHTML assignment.

[battre, 2013-12-14 00:24:58]

Hi Evan,

please take another look.

Thanks,
Dominic

https://codereview.chromium.org/108273002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/clear_browser_data_overlay.js (right):

https://codereview.chromium.org/108273002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/clear_browser_data_overlay.js:65:
stuffRemainsFooter.innerHTML.replace(/\[(.*)\]/,
On 2013/12/13 17:55:04, Evan Stade wrote:
> nit: can you get all the text before the first link, put it in a span, append
> the span, get all the text inside the first link, put it in a button, append
the
> button, etc.
> 
> In general it's preferable to avoid innerHTML assignment.

Done.

[Evan Stade, 2013-12-14 00:32:44]

lgtm, thanks

this is a lot of code, but I think there are some other places in settings that
could benefit from it as well. Hopefully they'll update their code/refactor this
new code to do so.

https://codereview.chromium.org/108273002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/108273002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7755: +        Saved [content settings] and
{search engines} will not be cleared and may reflect your browsing habits.
hmmm... it actually seems unlikely that a translation could transpose these two
links. Code would be a little simpler without multiple types of brackets.

[battre, 2013-12-14 00:47:52]

https://codereview.chromium.org/108273002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/108273002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7755: +        Saved [content settings] and
{search engines} will not be cleared and may reflect your browsing habits.
On 2013/12/14 00:32:45, Evan Stade wrote:
> hmmm... it actually seems unlikely that a translation could transpose these
two
> links. Code would be a little simpler without multiple types of brackets.

I think that the code would actually be slightly more complicated because we
need to count how many buttons have been added already.

Do you know whether the assumption that translatedText.indexOf('[') <
translatedText.indexOf(']') is maintained for RTL languages?

[Evan Stade, 2013-12-14 02:03:34]

That's a good point about RTL. I expect not.
On Dec 13, 2013 4:47 PM, <battre@chromium.org> wrote:

>
> https://codereview.chromium.org/108273002/diff/80001/chrome/app/generated_
> resources.grd
> File chrome/app/generated_resources.grd (right):
>
> https://codereview.chromium.org/108273002/diff/80001/chrome/app/generated_
> resources.grd#newcode7755
> chrome/app/generated_resources.grd:7755: +        Saved [content
> settings] and {search engines} will not be cleared and may reflect your
> browsing habits.
> On 2013/12/14 00:32:45, Evan Stade wrote:
>
>> hmmm... it actually seems unlikely that a translation could transpose
>>
> these two
>
>> links. Code would be a little simpler without multiple types of
>>
> brackets.
>
> I think that the code would actually be slightly more complicated
> because we need to count how many buttons have been added already.
>
> Do you know whether the assumption that translatedText.indexOf('[') <
> translatedText.indexOf(']') is maintained for RTL languages?
>
> https://codereview.chromium.org/108273002/
>

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

[battre, 2013-12-16 16:17:10]

I have replaced [] and {} with || and ## to be left-to-right/right-to-left
independent.

[battre, 2013-12-16 17:00:46]

I think the change is small enough that I will send this to the CQ so that we
hopefully make it for the branch point. We can continue tweaking this if you
would prefer to see other implementations.

[commit-bot: I haz the power, 2013-12-16 17:01:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/108273002/100001

[commit-bot: I haz the power, 2013-12-16 18:18:23]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 22:02:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/108273002/100001

[commit-bot: I haz the power, 2013-12-17 00:08:28]

Change committed as 241079