convert bookmark manager to loadTimeData from localStrings

convert bookmark manager to loadTimeData from localStrings

BUG=122753
TEST=manual

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

[Dan Beam, 2012-04-30 17:04:00]

https://chromiumcodereview.appspot.com/10244005/diff/2001/chrome/browser/reso...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/2001/chrome/browser/reso...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:542:
alert(loadTimeData.getString('invalid_url'));
every time an alert() box pops up a baby seal cries, :_(

https://chromiumcodereview.appspot.com/10244005/diff/2001/chrome/browser/reso...
File chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/2001/chrome/browser/reso...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:26:
process.src = "chrome://resources/js/i18n_template2.js";
single quotes

https://chromiumcodereview.appspot.com/10244005/diff/2001/chrome/browser/reso...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:34: const
ROOT_ID = '0';
you're gonna get a warning about using const here, btw

[Evan Stade, 2012-04-30 18:41:48]

http://codereview.chromium.org/10244005/diff/2001/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js (right):

http://codereview.chromium.org/10244005/diff/2001/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:26:
process.src = "chrome://resources/js/i18n_template2.js";
On 2012/04/30 17:04:00, Dan Beam wrote:
> single quotes

I confirmed with the author this file is no longer used, so I've reverted my
changes to it and later I'll just remove the file as a whole.

I did make the single quote change in main.js

http://codereview.chromium.org/10244005/diff/2001/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/bookmark_all_tabs.js:34: const
ROOT_ID = '0';
On 2012/04/30 17:04:00, Dan Beam wrote:
> you're gonna get a warning about using const here, btw

oh, just touching this file produces a lot of warnings, which I happily ignore.

[Evan Stade, 2012-05-03 00:16:02]

ping

[arv (Not doing code reviews), 2012-05-03 02:13:40]

https://chromiumcodereview.appspot.com/10244005/diff/7002/chrome/browser/reso...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/7002/chrome/browser/reso...
chrome/browser/resources/bookmark_manager/js/main.js:29: process.src =
'chrome://resources/js/i18n_template2.js';
I don't like. Why is this done in this way?

[Dan Beam, 2012-05-03 03:33:54]

https://chromiumcodereview.appspot.com/10244005/diff/7002/chrome/browser/reso...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/7002/chrome/browser/reso...
chrome/browser/resources/bookmark_manager/js/main.js:29: process.src =
'chrome://resources/js/i18n_template2.js';
On 2012/05/03 02:13:40, arv wrote:
> I don't like. Why is this done in this way?

I assumed because we're only setting the loadTimeData here rather than earlier
like in other scripts, so trying to process without any keys would result in
many errors.  Why we don't have the loadTimeData earlier is a different
question, though.

[Evan Stade, 2012-05-03 21:14:44]

http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/main.js:29: process.src =
'chrome://resources/js/i18n_template2.js';
On 2012/05/03 03:33:55, Dan Beam wrote:
> On 2012/05/03 02:13:40, arv wrote:
> > I don't like. Why is this done in this way?
> 
> I assumed because we're only setting the loadTimeData here rather than earlier
> like in other scripts, so trying to process without any keys would result in
> many errors.

yes. i18n_template2.js automatically processes.

>  Why we don't have the loadTimeData earlier is a different
> question, though.

because it has to be in this callback?

[arv (Not doing code reviews), 2012-05-03 22:02:05]

On 2012/05/03 21:14:44, Evan Stade wrote:
>
http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
> File chrome/browser/resources/bookmark_manager/js/main.js (right):
> 
>
http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
> chrome/browser/resources/bookmark_manager/js/main.js:29: process.src =
> 'chrome://resources/js/i18n_template2.js';
> On 2012/05/03 03:33:55, Dan Beam wrote:
> > On 2012/05/03 02:13:40, arv wrote:
> > > I don't like. Why is this done in this way?
> > 
> > I assumed because we're only setting the loadTimeData here rather than
earlier
> > like in other scripts, so trying to process without any keys would result in
> > many errors.
> 
> yes. i18n_template2.js automatically processes.
> 
> >  Why we don't have the loadTimeData earlier is a different
> > question, though.
> 
> because it has to be in this callback?

I would prefer that we change i18n_template2.js.

[Evan Stade, 2012-05-03 22:09:36]

On 2012/05/03 22:02:05, arv wrote:
> On 2012/05/03 21:14:44, Evan Stade wrote:
> >
>
http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
> > File chrome/browser/resources/bookmark_manager/js/main.js (right):
> > 
> >
>
http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
> > chrome/browser/resources/bookmark_manager/js/main.js:29: process.src =
> > 'chrome://resources/js/i18n_template2.js';
> > On 2012/05/03 03:33:55, Dan Beam wrote:
> > > On 2012/05/03 02:13:40, arv wrote:
> > > > I don't like. Why is this done in this way?
> > > 
> > > I assumed because we're only setting the loadTimeData here rather than
> earlier
> > > like in other scripts, so trying to process without any keys would result
in
> > > many errors.
> > 
> > yes. i18n_template2.js automatically processes.
> > 
> > >  Why we don't have the loadTimeData earlier is a different
> > > question, though.
> > 
> > because it has to be in this callback?
> 
> I would prefer that we change i18n_template2.js.

What drawbacks do you see with this approach? And do you have a particular
change in mind? I would not like to force every webui page to include two files
rather than one because of this single use case.

[arv (Not doing code reviews), 2012-05-03 22:22:17]

On Thu, May 3, 2012 at 3:09 PM,  <estade@chromium.org> wrote:
> On 2012/05/03 22:02:05, arv wrote:
>>
>> On 2012/05/03 21:14:44, Evan Stade wrote:
>> >
>
>
>
http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
>>
>> > File chrome/browser/resources/bookmark_manager/js/main.js (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/10244005/diff/7002/chrome/browser/resources/bo...
>>
>> > chrome/browser/resources/bookmark_manager/js/main.js:29: process.src =
>> > 'chrome://resources/js/i18n_template2.js';
>> > On 2012/05/03 03:33:55, Dan Beam wrote:
>> > > On 2012/05/03 02:13:40, arv wrote:
>> > > > I don't like. Why is this done in this way?
>> > >
>> > > I assumed because we're only setting the loadTimeData here rather than
>> earlier
>> > > like in other scripts, so trying to process without any keys would
>> > > result
>
> in
>>
>> > > many errors.
>> >
>> > yes. i18n_template2.js automatically processes.
>> >
>> > >  Why we don't have the loadTimeData earlier is a different
>> > > question, though.
>> >
>> > because it has to be in this callback?
>
>
>> I would prefer that we change i18n_template2.js.
>
>
> What drawbacks do you see with this approach? And do you have a particular
> change in mind? I would not like to force every webui page to include two
> files
> rather than one because of this single use case.
>
> http://codereview.chromium.org/10244005/

This is so hacky. We control all the code so we should do the right thing.

Some ideas:

1. Call process on DOMContentLoaded.
2. If loadData is not present when process is called we can register a
callback on the loadData.

-- 
erik

[Evan Stade, 2012-05-03 23:34:08]

how's this?

>
> Some ideas:
> 
> 1. Call process on DOMContentLoaded.

this would still be too early for bookmarks

> 2. If loadData is not present when process is called we can register a
> callback on the loadData.

sure, but that would make errors (not including loadTimeData at all) silent. I
think there is value in making it more obvious what the right thing to do is for
the 99% case.

[Evan Stade, 2012-05-08 22:30:26]

ping

[Dan Beam, 2012-05-08 22:34:25]

considering I thought of the same solution that you did independently, lol, lgtm
(w/nits)

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
chrome/browser/resources/bookmark_manager/js/main.js:1260: (linkController = new
cr.LinkController(loadTimeData));
nit: I think this could be less succinct but more readable.

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
File chrome/browser/resources/shared/js/i18n_template2.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
chrome/browser/resources/shared/js/i18n_template2.js:5: <include
src="./i18n_template_no_process.js"></include>
why not i18n_template2_no_process.js?

[Evan Stade, 2012-05-08 23:32:13]

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
chrome/browser/resources/bookmark_manager/js/main.js:1260: (linkController = new
cr.LinkController(loadTimeData));
On 2012/05/08 22:34:25, Dan Beam wrote:
> nit: I think this could be less succinct but more readable.

I was not the author, and I have no trouble reading it.

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
File chrome/browser/resources/shared/js/i18n_template2.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
chrome/browser/resources/shared/js/i18n_template2.js:5: <include
src="./i18n_template_no_process.js"></include>
On 2012/05/08 22:34:25, Dan Beam wrote:
> why not i18n_template2_no_process.js?

why should it be that? i18n_template2.js would be called i18n_template.js if
that were not already taken (for now).

[Dan Beam, 2012-05-08 23:46:50]

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
chrome/browser/resources/bookmark_manager/js/main.js:1260: (linkController = new
cr.LinkController(loadTimeData));
On 2012/05/08 23:32:13, Evan Stade wrote:
> On 2012/05/08 22:34:25, Dan Beam wrote:
> > nit: I think this could be less succinct but more readable.
> 
> I was not the author, and I have no trouble reading it.

I think it'd be more readable as:

  if (!linkController)
    linkController = new cr.LinkController(loadTimeData);

  return linkController;

We don't prefer this in C++ (we have to explicitly say if ((a = getNext())) {}),
so I don't know why we'd do it in JS.

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
File chrome/browser/resources/shared/js/i18n_template2.js (right):

https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
chrome/browser/resources/shared/js/i18n_template2.js:5: <include
src="./i18n_template_no_process.js"></include>
On 2012/05/08 23:32:13, Evan Stade wrote:
> On 2012/05/08 22:34:25, Dan Beam wrote:
> > why not i18n_template2_no_process.js?
> 
> why should it be that? i18n_template2.js would be called i18n_template.js if
> that were not already taken (for now).

because in the interim it sounds like you could include
i18n_template_no_process.js as a substitute for i18n_template.js (but not
i18n_template2.js). you can rename both i18n_content2.js and
i18n_content2_no_process.js together in a very short while, just as you will
anyways, IMO.

[Dan Beam, 2012-05-08 23:49:18]

On 2012/05/08 23:46:50, Dan Beam wrote:
>
https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
> File chrome/browser/resources/bookmark_manager/js/main.js (right):
> 
>
https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
> chrome/browser/resources/bookmark_manager/js/main.js:1260: (linkController =
new
> cr.LinkController(loadTimeData));
> On 2012/05/08 23:32:13, Evan Stade wrote:
> > On 2012/05/08 22:34:25, Dan Beam wrote:
> > > nit: I think this could be less succinct but more readable.
> > 
> > I was not the author, and I have no trouble reading it.
> 
> I think it'd be more readable as:
> 
>   if (!linkController)
>     linkController = new cr.LinkController(loadTimeData);
> 
>   return linkController;
> 
> We don't prefer this in C++ (we have to explicitly say if ((a = getNext()))
{}),
> so I don't know why we'd do it in JS.
> 
>
https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
> File chrome/browser/resources/shared/js/i18n_template2.js (right):
> 
>
https://chromiumcodereview.appspot.com/10244005/diff/23002/chrome/browser/res...
> chrome/browser/resources/shared/js/i18n_template2.js:5: <include
> src="./i18n_template_no_process.js"></include>
> On 2012/05/08 23:32:13, Evan Stade wrote:
> > On 2012/05/08 22:34:25, Dan Beam wrote:
> > > why not i18n_template2_no_process.js?
> > 
> > why should it be that? i18n_template2.js would be called i18n_template.js if
> > that were not already taken (for now).
> 
> because in the interim it sounds like you could include
> i18n_template_no_process.js as a substitute for i18n_template.js (but not
> i18n_template2.js). you can rename both i18n_content2.js and
> i18n_content2_no_process.js together in a very short while, just as you will
> anyways, IMO.

s/content/template/

[Evan Stade, 2012-05-09 23:16:54]

http://codereview.chromium.org/10244005/diff/23002/chrome/browser/resources/b...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

http://codereview.chromium.org/10244005/diff/23002/chrome/browser/resources/b...
chrome/browser/resources/bookmark_manager/js/main.js:1260: (linkController = new
cr.LinkController(loadTimeData));
On 2012/05/08 23:46:51, Dan Beam wrote:
> On 2012/05/08 23:32:13, Evan Stade wrote:
> > On 2012/05/08 22:34:25, Dan Beam wrote:
> > > nit: I think this could be less succinct but more readable.
> > 
> > I was not the author, and I have no trouble reading it.
> 
> I think it'd be more readable as:
> 
>   if (!linkController)
>     linkController = new cr.LinkController(loadTimeData);
> 
>   return linkController;
> 
> We don't prefer this in C++ (we have to explicitly say if ((a = getNext()))
{}),
> so I don't know why we'd do it in JS.

actually, it's not a matter of preference in C++, it simply doesn't work.
Luckily this is not C++.

[arv (Not doing code reviews), 2012-05-09 23:33:06]

LGTM

[commit-bot: I haz the power, 2012-05-09 23:41:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10244005/23002

[commit-bot: I haz the power, 2012-05-09 23:41:42]

Presubmit check for 10244005-23002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
See the JavaScript style guide at
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
and if you have any feedback about the JavaScript PRESUBMIT check, contact
tbreisacher@chromium.org

** Presubmit Warnings **
Found JavaScript style violations in
chrome/browser/resources/bookmark_manager/js/main.js:
  line 12: E0001: Extra space after "MenuButton"
/** @const */ var MenuButton  = cr.ui.MenuButton;
                            ^^
  line 34: E0010: Missing semicolon at end of line
    searchTreeItem.icon = 'images/bookmark_manager_search.png'
                                                             ^
  line 36: E0010: Missing semicolon at end of line
    searchTreeItem.icon = 'images/bookmark_manager_search_rtl.png'
                                                                 ^
  line 63: E0220: No docs found for member
'splitter.previousElementSibling.style.width'
  splitter.previousElementSibling.style.width = localStorage['treeWidth'];
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  line 204: E0012: Illegal semicolon after function declaration
};
 ^
  line 208: E0220: No docs found for member 'window.onhashchange'
window.onhashchange = function(e) {
^^^^^^^^^^^^^^^^^^^
  line 231: E0200: Invalid JsDoc tag: para
 * @para {string} searchText The text to search for.
   ^^^^^
  line 233: E0210: Missing docs for parameter: "searchText"
function setSearch(searchText) {
                             ^^
  line 352: E0217: Missing @return JsDoc in function with non-trivial return
   */
   ^^
  line 402: E0217: Missing @return JsDoc in function with non-trivial return
   */
   ^^
  line 453: E0217: Missing @return JsDoc in function with non-trivial return
   */
   ^^
  line 490: E0217: Missing @return JsDoc in function with non-trivial return
   */
   ^^
  line 656: E0010: Missing semicolon at end of line
        rect.width -= rect.left
                      ^^^^^^^^^
  line 824: E0220: No docs found for member 'list.contextMenu'
list.contextMenu = $('context-menu');
^^^^^^^^^^^^^^^^
  line 825: E0220: No docs found for member 'tree.contextMenu'
tree.contextMenu = $('context-menu');
^^^^^^^^^^^^^^^^
  line 832: E0220: No docs found for member 'command.disabled'
    command.disabled = true;
    ^^^^^^^^^^^^^^^^

Presubmit checks took 3.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!