Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
543863002:
Typecheck chrome://bookmarks using Closure Compiler (Closed)
|
Created:
6 years, 3 months ago by Vitaly Pavlenko Modified:
6 years, 2 months ago Reviewers:
Dan Beam CC:
chromium-reviews, extensions-reviews_chromium.org, tfarina, dcheng, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@true_master Project:
chromium Visibility:
Public. |
DescriptionTypecheck chrome://bookmarks using Closure Compiler
R=dbeam@chromium.org
BUG=393873
TEST=GYP_GENERATORS=ninja gyp --depth . chrome/browser/resources/bookmark_manager/js/compiled_resources.gyp && ninja -C out/Default
Committed: https://crrev.com/5d8e6e26496546a78df69aee799bbe3f60a8ee7f
Cr-Commit-Position: refs/heads/master@{#297560}
Patch Set 1 #Patch Set 2 : remove chrome_extensions.js #Patch Set 3 : rebase #
Total comments: 8
Patch Set 4 : revert checker.py #
Total comments: 36
Patch Set 5 : fixed comments #Patch Set 6 : some more errors #Patch Set 7 : remove runner.jar from CL #
Total comments: 10
Patch Set 8 : newest patchset #
Total comments: 13
Patch Set 9 : $('list') -> bmm.list #Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : fix test failure #Patch Set 13 : debug warns #Messages
Total messages: 61 (15 generated)
why is there a 7k+ line @externs file (that's not in third_party/closure_compiler/externs/)?
On 2014/09/06 03:16:28, Dan Beam wrote: > why is there a 7k+ line @externs file (that's not in > third_party/closure_compiler/externs/)? The issue is that I haven't placed it inside third_party/closure_compiler/externs/ directory, right? If so, it's my fault.
On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > On 2014/09/06 03:16:28, Dan Beam wrote: > > why is there a 7k+ line @externs file (that's not in > > third_party/closure_compiler/externs/)? > > The issue is that I haven't placed it inside > third_party/closure_compiler/externs/ directory, right? If so, it's my fault. why are we copying it to begin with? isn't it already in the externs?
On 2014/09/09 02:15:21, Dan Beam wrote: > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > why is there a 7k+ line @externs file (that's not in > > > third_party/closure_compiler/externs/)? > > > > The issue is that I haven't placed it inside > > third_party/closure_compiler/externs/ directory, right? If so, it's my fault. > > why are we copying it to begin with? isn't it already in the externs? CC tbreisacher@ If I understand correctly, there is no contrib/externs/chrome_extensions.js in compiler.jar, so no externs are shipped with the compiler, and therefore there's no easy way to use them, even if they are present in the Compiler source tree. Sounds weird, so it's very likely that I'm missing something. tbreisacher@, could you explain how one should use them?
On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > On 2014/09/09 02:15:21, Dan Beam wrote: > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > why is there a 7k+ line @externs file (that's not in > > > > third_party/closure_compiler/externs/)? > > > > > > The issue is that I haven't placed it inside > > > third_party/closure_compiler/externs/ directory, right? If so, it's my > fault. > > > > why are we copying it to begin with? isn't it already in the externs? > > CC tbreisacher@ > > If I understand correctly, there is no contrib/externs/chrome_extensions.js in > compiler.jar, so no externs are shipped with the compiler, and therefore there's > no easy way to use them, even if they are present in the Compiler source tree. > Sounds weird, so it's very likely that I'm missing something. tbreisacher@, > could you explain how one should use them? Right. They're in the source tree (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) but when you build the jar with 'ant jar' (or download it from dl.google.com) you get only the default externs. You could add the entire Closure Compiler source tree as a dependency but that might be overkill. Unfortunately it will be somewhat of a pain keeping your copy of chrome_extensions.js in sync with the compiler's but I don't know if there's a great solution to that.
On 2014/09/09 17:24:37, Tyler Breisacher (Chromium) wrote: > On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > > On 2014/09/09 02:15:21, Dan Beam wrote: > > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > > why is there a 7k+ line @externs file (that's not in > > > > > third_party/closure_compiler/externs/)? > > > > > > > > The issue is that I haven't placed it inside > > > > third_party/closure_compiler/externs/ directory, right? If so, it's my > > fault. > > > > > > why are we copying it to begin with? isn't it already in the externs? > > > > CC tbreisacher@ > > > > If I understand correctly, there is no contrib/externs/chrome_extensions.js in > > compiler.jar, so no externs are shipped with the compiler, and therefore > there's > > no easy way to use them, even if they are present in the Compiler source tree. > > Sounds weird, so it's very likely that I'm missing something. tbreisacher@, > > could you explain how one should use them? > > Right. They're in the source tree > (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) > but when you build the jar with 'ant jar' (or download it from http://dl.google.com) > you get only the default externs. > > You could add the entire Closure Compiler source tree as a dependency but that > might be overkill. Unfortunately it will be somewhat of a pain keeping your copy > of chrome_extensions.js in sync with the compiler's but I don't know if there's > a great solution to that. we already build compiler.jar from source. is there a target or an easy way to get a .jar with the contrib externs as well?
On 2014/09/10 19:28:10, Dan Beam wrote: > On 2014/09/09 17:24:37, Tyler Breisacher (Chromium) wrote: > > On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > > > On 2014/09/09 02:15:21, Dan Beam wrote: > > > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > > > why is there a 7k+ line @externs file (that's not in > > > > > > third_party/closure_compiler/externs/)? > > > > > > > > > > The issue is that I haven't placed it inside > > > > > third_party/closure_compiler/externs/ directory, right? If so, it's my > > > fault. > > > > > > > > why are we copying it to begin with? isn't it already in the externs? > > > > > > CC tbreisacher@ > > > > > > If I understand correctly, there is no contrib/externs/chrome_extensions.js > in > > > compiler.jar, so no externs are shipped with the compiler, and therefore > > there's > > > no easy way to use them, even if they are present in the Compiler source > tree. > > > Sounds weird, so it's very likely that I'm missing something. tbreisacher@, > > > could you explain how one should use them? > > > > Right. They're in the source tree > > > (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) > > but when you build the jar with 'ant jar' (or download it from > http://dl.google.com) > > you get only the default externs. > > > > You could add the entire Closure Compiler source tree as a dependency but that > > might be overkill. Unfortunately it will be somewhat of a pain keeping your > copy > > of chrome_extensions.js in sync with the compiler's but I don't know if > there's > > a great solution to that. > > we already build compiler.jar from source. is there a target or an easy way to > get a .jar with the contrib externs as well? I see no such target: vitalyp@vitalyp0:~/closure-compiler$ cat build.xml | grep contrib vitalyp@vitalyp0:~/closure-compiler$
On 2014/09/12 00:21:57, Vitaly Pavlenko wrote: > On 2014/09/10 19:28:10, Dan Beam wrote: > > On 2014/09/09 17:24:37, Tyler Breisacher (Chromium) wrote: > > > On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > > > > On 2014/09/09 02:15:21, Dan Beam wrote: > > > > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > > > > why is there a 7k+ line @externs file (that's not in > > > > > > > third_party/closure_compiler/externs/)? > > > > > > > > > > > > The issue is that I haven't placed it inside > > > > > > third_party/closure_compiler/externs/ directory, right? If so, it's my > > > > fault. > > > > > > > > > > why are we copying it to begin with? isn't it already in the externs? > > > > > > > > CC tbreisacher@ > > > > > > > > If I understand correctly, there is no > contrib/externs/chrome_extensions.js > > in > > > > compiler.jar, so no externs are shipped with the compiler, and therefore > > > there's > > > > no easy way to use them, even if they are present in the Compiler source > > tree. > > > > Sounds weird, so it's very likely that I'm missing something. > tbreisacher@, > > > > could you explain how one should use them? > > > > > > Right. They're in the source tree > > > > > > (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) > > > but when you build the jar with 'ant jar' (or download it from > > http://dl.google.com) > > > you get only the default externs. > > > > > > You could add the entire Closure Compiler source tree as a dependency but > that > > > might be overkill. Unfortunately it will be somewhat of a pain keeping your > > copy > > > of chrome_extensions.js in sync with the compiler's but I don't know if > > there's > > > a great solution to that. > > > > we already build compiler.jar from source. is there a target or an easy way > to > > get a .jar with the contrib externs as well? > > I see no such target: > > vitalyp@vitalyp0:~/closure-compiler$ cat build.xml | grep contrib > vitalyp@vitalyp0:~/closure-compiler$ Ping dbeam@, tbreisacher@. If there's no chance to use externs from Closure Compiler source, what should we do? Maybe I can add a target to Closure Compiler build which includes the sources into jar file, and add a compiler option to use internal externs. And we will use this target every time we build Closure Compiler for Chrome.
On 2014/09/12 22:28:25, Vitaly Pavlenko wrote: > On 2014/09/12 00:21:57, Vitaly Pavlenko wrote: > > On 2014/09/10 19:28:10, Dan Beam wrote: > > > On 2014/09/09 17:24:37, Tyler Breisacher (Chromium) wrote: > > > > On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > > > > > On 2014/09/09 02:15:21, Dan Beam wrote: > > > > > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > > > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > > > > > why is there a 7k+ line @externs file (that's not in > > > > > > > > third_party/closure_compiler/externs/)? > > > > > > > > > > > > > > The issue is that I haven't placed it inside > > > > > > > third_party/closure_compiler/externs/ directory, right? If so, it's > my > > > > > fault. > > > > > > > > > > > > why are we copying it to begin with? isn't it already in the externs? > > > > > > > > > > CC tbreisacher@ > > > > > > > > > > If I understand correctly, there is no > > contrib/externs/chrome_extensions.js > > > in > > > > > compiler.jar, so no externs are shipped with the compiler, and therefore > > > > there's > > > > > no easy way to use them, even if they are present in the Compiler source > > > tree. > > > > > Sounds weird, so it's very likely that I'm missing something. > > tbreisacher@, > > > > > could you explain how one should use them? > > > > > > > > Right. They're in the source tree > > > > > > > > > > (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) > > > > but when you build the jar with 'ant jar' (or download it from > > > http://dl.google.com) > > > > you get only the default externs. > > > > > > > > You could add the entire Closure Compiler source tree as a dependency but > > that > > > > might be overkill. Unfortunately it will be somewhat of a pain keeping > your > > > copy > > > > of chrome_extensions.js in sync with the compiler's but I don't know if > > > there's > > > > a great solution to that. > > > > > > we already build compiler.jar from source. is there a target or an easy way > > to > > > get a .jar with the contrib externs as well? > > > > I see no such target: > > > > vitalyp@vitalyp0:~/closure-compiler$ cat build.xml | grep contrib > > vitalyp@vitalyp0:~/closure-compiler$ > > Ping dbeam@, tbreisacher@. If there's no chance to use externs from Closure > Compiler source, what should we do? Maybe I can add a target to Closure Compiler > build which includes the sources into jar file, and add a compiler option to use > internal externs. And we will use this target every time we build Closure > Compiler for Chrome. i think other parts of the community would benefit from having a jar-more-externs or jar-contrib target so if we can add a target to closure's build.xml that'd be better, IMO
On 2014/09/12 23:26:30, Dan Beam wrote: > On 2014/09/12 22:28:25, Vitaly Pavlenko wrote: > > On 2014/09/12 00:21:57, Vitaly Pavlenko wrote: > > > On 2014/09/10 19:28:10, Dan Beam wrote: > > > > On 2014/09/09 17:24:37, Tyler Breisacher (Chromium) wrote: > > > > > On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > > > > > > On 2014/09/09 02:15:21, Dan Beam wrote: > > > > > > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > > > > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > > > > > > why is there a 7k+ line @externs file (that's not in > > > > > > > > > third_party/closure_compiler/externs/)? > > > > > > > > > > > > > > > > The issue is that I haven't placed it inside > > > > > > > > third_party/closure_compiler/externs/ directory, right? If so, > it's > > my > > > > > > fault. > > > > > > > > > > > > > > why are we copying it to begin with? isn't it already in the > externs? > > > > > > > > > > > > CC tbreisacher@ > > > > > > > > > > > > If I understand correctly, there is no > > > contrib/externs/chrome_extensions.js > > > > in > > > > > > compiler.jar, so no externs are shipped with the compiler, and > therefore > > > > > there's > > > > > > no easy way to use them, even if they are present in the Compiler > source > > > > tree. > > > > > > Sounds weird, so it's very likely that I'm missing something. > > > tbreisacher@, > > > > > > could you explain how one should use them? > > > > > > > > > > Right. They're in the source tree > > > > > > > > > > > > > > > (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) > > > > > but when you build the jar with 'ant jar' (or download it from > > > > http://dl.google.com) > > > > > you get only the default externs. > > > > > > > > > > You could add the entire Closure Compiler source tree as a dependency > but > > > that > > > > > might be overkill. Unfortunately it will be somewhat of a pain keeping > > your > > > > copy > > > > > of chrome_extensions.js in sync with the compiler's but I don't know if > > > > there's > > > > > a great solution to that. > > > > > > > > we already build compiler.jar from source. is there a target or an easy > way > > > to > > > > get a .jar with the contrib externs as well? > > > > > > I see no such target: > > > > > > vitalyp@vitalyp0:~/closure-compiler$ cat build.xml | grep contrib > > > vitalyp@vitalyp0:~/closure-compiler$ > > > > Ping dbeam@, tbreisacher@. If there's no chance to use externs from Closure > > Compiler source, what should we do? Maybe I can add a target to Closure > Compiler > > build which includes the sources into jar file, and add a compiler option to > use > > internal externs. And we will use this target every time we build Closure > > Compiler for Chrome. > > i think other parts of the community would benefit from having a > jar-more-externs or jar-contrib target so if we can add a target to closure's > build.xml that'd be better, IMO Ping tbreisacher@, should I start to do that? Should I discuss the way I'm going to do that somewhere with compiler team?
On 2014/09/15 17:21:17, Vitaly Pavlenko wrote: > On 2014/09/12 23:26:30, Dan Beam wrote: > > On 2014/09/12 22:28:25, Vitaly Pavlenko wrote: > > > On 2014/09/12 00:21:57, Vitaly Pavlenko wrote: > > > > On 2014/09/10 19:28:10, Dan Beam wrote: > > > > > On 2014/09/09 17:24:37, Tyler Breisacher (Chromium) wrote: > > > > > > On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > > > > > > > On 2014/09/09 02:15:21, Dan Beam wrote: > > > > > > > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > > > > > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > > > > > > > why is there a 7k+ line @externs file (that's not in > > > > > > > > > > third_party/closure_compiler/externs/)? > > > > > > > > > > > > > > > > > > The issue is that I haven't placed it inside > > > > > > > > > third_party/closure_compiler/externs/ directory, right? If so, > > it's > > > my > > > > > > > fault. > > > > > > > > > > > > > > > > why are we copying it to begin with? isn't it already in the > > externs? > > > > > > > > > > > > > > CC tbreisacher@ > > > > > > > > > > > > > > If I understand correctly, there is no > > > > contrib/externs/chrome_extensions.js > > > > > in > > > > > > > compiler.jar, so no externs are shipped with the compiler, and > > therefore > > > > > > there's > > > > > > > no easy way to use them, even if they are present in the Compiler > > source > > > > > tree. > > > > > > > Sounds weird, so it's very likely that I'm missing something. > > > > tbreisacher@, > > > > > > > could you explain how one should use them? > > > > > > > > > > > > Right. They're in the source tree > > > > > > > > > > > > > > > > > > > > > (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) > > > > > > but when you build the jar with 'ant jar' (or download it from > > > > > http://dl.google.com) > > > > > > you get only the default externs. > > > > > > > > > > > > You could add the entire Closure Compiler source tree as a dependency > > but > > > > that > > > > > > might be overkill. Unfortunately it will be somewhat of a pain keeping > > > your > > > > > copy > > > > > > of chrome_extensions.js in sync with the compiler's but I don't know > if > > > > > there's > > > > > > a great solution to that. > > > > > > > > > > we already build compiler.jar from source. is there a target or an easy > > way > > > > to > > > > > get a .jar with the contrib externs as well? > > > > > > > > I see no such target: > > > > > > > > vitalyp@vitalyp0:~/closure-compiler$ cat build.xml | grep contrib > > > > vitalyp@vitalyp0:~/closure-compiler$ > > > > > > Ping dbeam@, tbreisacher@. If there's no chance to use externs from Closure > > > Compiler source, what should we do? Maybe I can add a target to Closure > > Compiler > > > build which includes the sources into jar file, and add a compiler option to > > use > > > internal externs. And we will use this target every time we build Closure > > > Compiler for Chrome. > > > > i think other parts of the community would benefit from having a > > jar-more-externs or jar-contrib target so if we can add a target to closure's > > build.xml that'd be better, IMO > > Ping tbreisacher@, should I start to do that? Should I discuss the way I'm going > to do that somewhere with compiler team? ping tbreisacher@
On 2014/09/16 17:57:35, Vitaly Pavlenko wrote: > On 2014/09/15 17:21:17, Vitaly Pavlenko wrote: > > On 2014/09/12 23:26:30, Dan Beam wrote: > > > On 2014/09/12 22:28:25, Vitaly Pavlenko wrote: > > > > On 2014/09/12 00:21:57, Vitaly Pavlenko wrote: > > > > > On 2014/09/10 19:28:10, Dan Beam wrote: > > > > > > On 2014/09/09 17:24:37, Tyler Breisacher (Chromium) wrote: > > > > > > > On 2014/09/09 17:13:19, Vitaly Pavlenko wrote: > > > > > > > > On 2014/09/09 02:15:21, Dan Beam wrote: > > > > > > > > > On 2014/09/06 23:16:06, Vitaly Pavlenko wrote: > > > > > > > > > > On 2014/09/06 03:16:28, Dan Beam wrote: > > > > > > > > > > > why is there a 7k+ line @externs file (that's not in > > > > > > > > > > > third_party/closure_compiler/externs/)? > > > > > > > > > > > > > > > > > > > > The issue is that I haven't placed it inside > > > > > > > > > > third_party/closure_compiler/externs/ directory, right? If so, > > > it's > > > > my > > > > > > > > fault. > > > > > > > > > > > > > > > > > > why are we copying it to begin with? isn't it already in the > > > externs? > > > > > > > > > > > > > > > > CC tbreisacher@ > > > > > > > > > > > > > > > > If I understand correctly, there is no > > > > > contrib/externs/chrome_extensions.js > > > > > > in > > > > > > > > compiler.jar, so no externs are shipped with the compiler, and > > > therefore > > > > > > > there's > > > > > > > > no easy way to use them, even if they are present in the Compiler > > > source > > > > > > tree. > > > > > > > > Sounds weird, so it's very likely that I'm missing something. > > > > > tbreisacher@, > > > > > > > > could you explain how one should use them? > > > > > > > > > > > > > > Right. They're in the source tree > > > > > > > > > > > > > > > > > > > > > > > > > > > > (https://github.com/google/closure-compiler/blob/master/contrib/externs/chrome...) > > > > > > > but when you build the jar with 'ant jar' (or download it from > > > > > > http://dl.google.com) > > > > > > > you get only the default externs. > > > > > > > > > > > > > > You could add the entire Closure Compiler source tree as a > dependency > > > but > > > > > that > > > > > > > might be overkill. Unfortunately it will be somewhat of a pain > keeping > > > > your > > > > > > copy > > > > > > > of chrome_extensions.js in sync with the compiler's but I don't know > > if > > > > > > there's > > > > > > > a great solution to that. > > > > > > > > > > > > we already build compiler.jar from source. is there a target or an > easy > > > way > > > > > to > > > > > > get a .jar with the contrib externs as well? > > > > > > > > > > I see no such target: > > > > > > > > > > vitalyp@vitalyp0:~/closure-compiler$ cat build.xml | grep contrib > > > > > vitalyp@vitalyp0:~/closure-compiler$ > > > > > > > > Ping dbeam@, tbreisacher@. If there's no chance to use externs from > Closure > > > > Compiler source, what should we do? Maybe I can add a target to Closure > > > Compiler > > > > build which includes the sources into jar file, and add a compiler option > to > > > use > > > > internal externs. And we will use this target every time we build Closure > > > > Compiler for Chrome. > > > > > > i think other parts of the community would benefit from having a > > > jar-more-externs or jar-contrib target so if we can add a target to > closure's > > > build.xml that'd be better, IMO > > > > Ping tbreisacher@, should I start to do that? Should I discuss the way I'm > going > > to do that somewhere with compiler team? > > ping tbreisacher@ Added script to update compiler.jar and chrome_extensions.js: https://chromiumcodereview.appspot.com/582193002/ dbeam@, please, review this CL.
On 2014/09/18 20:52:25, Vitaly Pavlenko wrote: > Added script to update compiler.jar and chrome_extensions.js: > https://chromiumcodereview.appspot.com/582193002/ > > dbeam@, please, review this CL. ^ split this into separate CL
On 2014/09/18 21:10:47, Dan Beam wrote: > On 2014/09/18 20:52:25, Vitaly Pavlenko wrote: > > Added script to update compiler.jar and chrome_extensions.js: > > https://chromiumcodereview.appspot.com/582193002/ > > > > dbeam@, please, review this CL. > > ^ split this into separate CL Removed chrome_extensions.js. Should I split out something else?
https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... File third_party/closure_compiler/checker.py (right): https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/checker.py:63: self._chrome_extensions = os.path.join(current_dir, "chrome_extensions.js") include when needed in gyp rather than always https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/checker.py:174: externs = [self._chrome_extensions] + externs externs += [self._chrome_extensions] https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... File third_party/closure_compiler/externs/bookmark_manager_private.js (right): https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/externs/bookmark_manager_private.js:3: // found in the LICENSE file. is this from closure or did you write this? https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... File third_party/closure_compiler/externs/metrics_private.js (right): https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/externs/metrics_private.js:3: // found in the LICENSE file. same question
https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... File third_party/closure_compiler/checker.py (right): https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/checker.py:63: self._chrome_extensions = os.path.join(current_dir, "chrome_extensions.js") On 2014/09/18 23:13:12, Dan Beam wrote: > include when needed in gyp rather than always Done. https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/checker.py:174: externs = [self._chrome_extensions] + externs On 2014/09/18 23:13:12, Dan Beam wrote: > externs += [self._chrome_extensions] Removed. https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... File third_party/closure_compiler/externs/bookmark_manager_private.js (right): https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/externs/bookmark_manager_private.js:3: // found in the LICENSE file. On 2014/09/18 23:13:12, Dan Beam wrote: > is this from closure or did you write this? This file is generated by me using this script: https://chromiumcodereview.appspot.com/511943003/ and this source: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... I added the LICENSE string to the script used for generation. https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... File third_party/closure_compiler/externs/metrics_private.js (right): https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... third_party/closure_compiler/externs/metrics_private.js:3: // found in the LICENSE file. On 2014/09/18 23:13:12, Dan Beam wrote: > same question Same answer, source: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
On 2014/09/19 00:48:46, Vitaly Pavlenko wrote: > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > File third_party/closure_compiler/checker.py (right): > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > third_party/closure_compiler/checker.py:63: self._chrome_extensions = > os.path.join(current_dir, "chrome_extensions.js") > On 2014/09/18 23:13:12, Dan Beam wrote: > > include when needed in gyp rather than always > > Done. > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > third_party/closure_compiler/checker.py:174: externs = [self._chrome_extensions] > + externs > On 2014/09/18 23:13:12, Dan Beam wrote: > > externs += [self._chrome_extensions] > > Removed. > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > File third_party/closure_compiler/externs/bookmark_manager_private.js (right): > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > third_party/closure_compiler/externs/bookmark_manager_private.js:3: // found in > the LICENSE file. > On 2014/09/18 23:13:12, Dan Beam wrote: > > is this from closure or did you write this? > > This file is generated by me using this script: > https://chromiumcodereview.appspot.com/511943003/ > and this source: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > I added the LICENSE string to the script used for generation. > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > File third_party/closure_compiler/externs/metrics_private.js (right): > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > third_party/closure_compiler/externs/metrics_private.js:3: // found in the > LICENSE file. > On 2014/09/18 23:13:12, Dan Beam wrote: > > same question > > Same answer, source: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... ping dbeam@
On 2014/09/19 20:23:41, Vitaly Pavlenko wrote: > On 2014/09/19 00:48:46, Vitaly Pavlenko wrote: > > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > > File third_party/closure_compiler/checker.py (right): > > > > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > > third_party/closure_compiler/checker.py:63: self._chrome_extensions = > > os.path.join(current_dir, "chrome_extensions.js") > > On 2014/09/18 23:13:12, Dan Beam wrote: > > > include when needed in gyp rather than always > > > > Done. > > > > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > > third_party/closure_compiler/checker.py:174: externs = > [self._chrome_extensions] > > + externs > > On 2014/09/18 23:13:12, Dan Beam wrote: > > > externs += [self._chrome_extensions] > > > > Removed. > > > > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > > File third_party/closure_compiler/externs/bookmark_manager_private.js (right): > > > > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > > third_party/closure_compiler/externs/bookmark_manager_private.js:3: // found > in > > the LICENSE file. > > On 2014/09/18 23:13:12, Dan Beam wrote: > > > is this from closure or did you write this? > > > > This file is generated by me using this script: > > https://chromiumcodereview.appspot.com/511943003/ > > and this source: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > > > I added the LICENSE string to the script used for generation. > > > > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > > File third_party/closure_compiler/externs/metrics_private.js (right): > > > > > https://chromiumcodereview.appspot.com/543863002/diff/40001/third_party/closu... > > third_party/closure_compiler/externs/metrics_private.js:3: // found in the > > LICENSE file. > > On 2014/09/18 23:13:12, Dan Beam wrote: > > > same question > > > > Same answer, source: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > ping dbeam@ ping dbeam@
https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:31: * @suppress {suspiciousCode} why can't we simply change the module return statement to: return { ... list: $('list'), }; instead? https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:561: }; why did you make this change? https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:21: * @suppress {suspiciousCode} same https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:33: window.localStorage['bookmarkTreeState']) why can't we just make an alias, e.g. var localStorage = window.localStorage; so we don't have to rewrite to window. everywhere? https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:151: ).map(function(item) { use a temporary variable to keep the code pretty https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:284: parentTreeItem.add(item); var children = assert(bookmarkNode.children); var anyChildren = buildTreeItems(item, children); https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/dnd.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/dnd.js:197: * @type {Function} nit: ?Function https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/dnd.js:211: return $('list').isSearch() && $('list').contains(overElement); i think for now it'd probably be better to do: var list = $('list'); at the top of the file (same with tree) if that works. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:164: if ($('list').parentId == id) cast to !Element here as you're already doing maybeNull.property https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:168: opt_callback); if () needs curlies https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/context_menu_button.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/context_menu_button.js:24: return assertInstanceof(el, HTMLElement); what happens when el is null? https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/context_menu_handler.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/context_menu_handler.js:219: addContextMenuProperty: function(element_or_class) { elementOrClass https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/context_menu_handler.js:220: var target = (typeof element_or_class == 'function') ? no ()s needed https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/list.js (left): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/list.js:526: if (!this.contains(e.relatedTarget)) why did you remove? https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/touch_handler.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/touch_handler.js:841: * @param {!Element} touchedElement why does this need to be non-null? https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/tree.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/tree.js:598: var onFocus = function() { why this change? also, ; after function expressions https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/tree.js:656: if (nextSibling) { nit: no curlies
https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:31: * @suppress {suspiciousCode} On 2014/09/23 02:46:55, Dan Beam wrote: > why can't we simply change the module return statement to: > > return { > ... > list: $('list'), > }; > > instead? Done, fixed handling in compiler pass. But I don't think it's a good idea: at the point when return object is created DOM is not processed, so $('list') returns null. As long as it's not used before it's assigned externally: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... it's ok, but it could mislead future code readers. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:561: }; On 2014/09/23 02:46:55, Dan Beam wrote: > why did you make this change? ## /usr/local/google/home/vitalyp/src/chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:562: ERROR - functions can only be declared at top level or immediately within another function in ES5 strict mode ## function stopPropagation(e) { ## ^ ## https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:21: * @suppress {suspiciousCode} On 2014/09/23 02:46:55, Dan Beam wrote: > same Done. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:33: window.localStorage['bookmarkTreeState']) On 2014/09/23 02:46:55, Dan Beam wrote: > why can't we just make an alias, e.g. > > var localStorage = window.localStorage; > > so we don't have to rewrite to window. everywhere? Done. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:151: ).map(function(item) { On 2014/09/23 02:46:55, Dan Beam wrote: > use a temporary variable to keep the code pretty Done. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:284: parentTreeItem.add(item); On 2014/09/23 02:46:55, Dan Beam wrote: > var children = assert(bookmarkNode.children); > var anyChildren = buildTreeItems(item, children); Done. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/dnd.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/dnd.js:197: * @type {Function} On 2014/09/23 02:46:55, Dan Beam wrote: > nit: ?Function Done. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/dnd.js:211: return $('list').isSearch() && $('list').contains(overElement); On 2014/09/23 02:46:56, Dan Beam wrote: > i think for now it'd probably be better to do: > > var list = $('list'); > > at the top of the file (same with tree) if that works. Nah, it doesn't work: at the time when the top of the file is processed DOM is not ready yet, apparently. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:164: if ($('list').parentId == id) On 2014/09/23 02:46:56, Dan Beam wrote: > cast to !Element here as you're already doing maybeNull.property Done. https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:168: opt_callback); On 2014/09/23 02:46:56, Dan Beam wrote: > if () needs curlies Done. https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/context_menu_button.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/context_menu_button.js:24: return assertInstanceof(el, HTMLElement); On 2014/09/23 02:46:56, Dan Beam wrote: > what happens when el is null? Done. https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/context_menu_handler.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/context_menu_handler.js:219: addContextMenuProperty: function(element_or_class) { On 2014/09/23 02:46:56, Dan Beam wrote: > elementOrClass Done. https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/context_menu_handler.js:220: var target = (typeof element_or_class == 'function') ? On 2014/09/23 02:46:56, Dan Beam wrote: > no ()s needed Done. https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/list.js (left): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/list.js:526: if (!this.contains(e.relatedTarget)) On 2014/09/23 02:46:56, Dan Beam wrote: > why did you remove? Based on this list, blur can't have relatedTarget: https://developer.mozilla.org/en-US/docs/Web/API/event.relatedTarget https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/touch_handler.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/touch_handler.js:841: * @param {!Element} touchedElement On 2014/09/23 02:46:56, Dan Beam wrote: > why does this need to be non-null? Because TouchHandler.Event() constructor waits for non-null: https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/tree.js (right): https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/tree.js:598: var onFocus = function() { On 2014/09/23 02:46:56, Dan Beam wrote: > why this change? also, ; after function expressions Why: ERROR - functions can only be declared at top level or immediately within another function in ES5 strict mode Semicolon: done. https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/tree.js:656: if (nextSibling) { On 2014/09/23 02:46:56, Dan Beam wrote: > nit: no curlies Done.
On 2014/09/23 22:20:56, Vitaly Pavlenko wrote: > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:31: * > @suppress {suspiciousCode} > On 2014/09/23 02:46:55, Dan Beam wrote: > > why can't we simply change the module return statement to: > > > > return { > > ... > > list: $('list'), > > }; > > > > instead? > > Done, fixed handling in compiler pass. But I don't think it's a good idea: at > the point when return object is created DOM is not processed, so $('list') > returns null. > > As long as it's not used before it's assigned externally: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > it's ok, but it could mislead future code readers. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:561: }; > On 2014/09/23 02:46:55, Dan Beam wrote: > > why did you make this change? > > ## > /usr/local/google/home/vitalyp/src/chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:562: > ERROR - functions can only be declared at top level or immediately within > another function in ES5 strict mode > ## function stopPropagation(e) { > ## ^ > ## > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right): > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:21: * > @suppress {suspiciousCode} > On 2014/09/23 02:46:55, Dan Beam wrote: > > same > > Done. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:33: > window.localStorage['bookmarkTreeState']) > On 2014/09/23 02:46:55, Dan Beam wrote: > > why can't we just make an alias, e.g. > > > > var localStorage = window.localStorage; > > > > so we don't have to rewrite to window. everywhere? > > Done. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:151: > ).map(function(item) { > On 2014/09/23 02:46:55, Dan Beam wrote: > > use a temporary variable to keep the code pretty > > Done. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:284: > parentTreeItem.add(item); > On 2014/09/23 02:46:55, Dan Beam wrote: > > var children = assert(bookmarkNode.children); > > var anyChildren = buildTreeItems(item, children); > > Done. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > File chrome/browser/resources/bookmark_manager/js/dnd.js (right): > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/dnd.js:197: * @type {Function} > On 2014/09/23 02:46:55, Dan Beam wrote: > > nit: ?Function > > Done. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/dnd.js:211: return > $('list').isSearch() && $('list').contains(overElement); > On 2014/09/23 02:46:56, Dan Beam wrote: > > i think for now it'd probably be better to do: > > > > var list = $('list'); > > > > at the top of the file (same with tree) if that works. > > Nah, it doesn't work: at the time when the top of the file is processed DOM is > not ready yet, apparently. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > File chrome/browser/resources/bookmark_manager/js/main.js (right): > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/main.js:164: if ($('list').parentId > == id) > On 2014/09/23 02:46:56, Dan Beam wrote: > > cast to !Element here as you're already doing maybeNull.property > > Done. > > https://codereview.chromium.org/543863002/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/main.js:168: opt_callback); > On 2014/09/23 02:46:56, Dan Beam wrote: > > if () needs curlies > > Done. > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > File ui/webui/resources/js/cr/ui/context_menu_button.js (right): > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/context_menu_button.js:24: return > assertInstanceof(el, HTMLElement); > On 2014/09/23 02:46:56, Dan Beam wrote: > > what happens when el is null? > > Done. > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > File ui/webui/resources/js/cr/ui/context_menu_handler.js (right): > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/context_menu_handler.js:219: addContextMenuProperty: > function(element_or_class) { > On 2014/09/23 02:46:56, Dan Beam wrote: > > elementOrClass > > Done. > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/context_menu_handler.js:220: var target = (typeof > element_or_class == 'function') ? > On 2014/09/23 02:46:56, Dan Beam wrote: > > no ()s needed > > Done. > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > File ui/webui/resources/js/cr/ui/list.js (left): > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/list.js:526: if (!this.contains(e.relatedTarget)) > On 2014/09/23 02:46:56, Dan Beam wrote: > > why did you remove? > > Based on this list, blur can't have relatedTarget: > https://developer.mozilla.org/en-US/docs/Web/API/event.relatedTarget > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > File ui/webui/resources/js/cr/ui/touch_handler.js (right): > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/touch_handler.js:841: * @param {!Element} > touchedElement > On 2014/09/23 02:46:56, Dan Beam wrote: > > why does this need to be non-null? > > Because TouchHandler.Event() constructor waits for non-null: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > File ui/webui/resources/js/cr/ui/tree.js (right): > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/tree.js:598: var onFocus = function() { > On 2014/09/23 02:46:56, Dan Beam wrote: > > why this change? also, ; after function expressions > > Why: > ERROR - functions can only be declared at top level or immediately within > another function in ES5 strict mode > > Semicolon: done. > > https://codereview.chromium.org/543863002/diff/60001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/tree.js:656: if (nextSibling) { > On 2014/09/23 02:46:56, Dan Beam wrote: > > nit: no curlies > > Done. ping dbeam@
https://chromiumcodereview.appspot.com/543863002/diff/60001/chrome/browser/re... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/60001/chrome/browser/re... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:31: * @suppress {suspiciousCode} On 2014/09/23 22:20:54, Vitaly Pavlenko wrote: > On 2014/09/23 02:46:55, Dan Beam wrote: > > why can't we simply change the module return statement to: > > > > return { > > ... > > list: $('list'), > > }; > > > > instead? > > Done, fixed handling in compiler pass. But I don't think it's a good idea: at > the point when return object is created DOM is not processed, so $('list') > returns null. > > As long as it's not used before it's assigned externally: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > it's ok, but it could mislead future code readers. see below https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:631: list: $('list'), either: get list() { return $('list'); }, or document.addEventListener('DOMContentLoaded', function() { bmm.list = $('list'); }); https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:16: /** @const */ var localStorage = window.localStorage; this is not @const (you're doing localStorage['blah'] = data), how does this work? does @const do nothing? https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:165: if (list.parentId == id) { no curlies now
https://chromiumcodereview.appspot.com/543863002/diff/60001/chrome/browser/re... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/60001/chrome/browser/re... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:31: * @suppress {suspiciousCode} On 2014/09/24 21:38:58, Dan Beam wrote: > On 2014/09/23 22:20:54, Vitaly Pavlenko wrote: > > On 2014/09/23 02:46:55, Dan Beam wrote: > > > why can't we simply change the module return statement to: > > > > > > return { > > > ... > > > list: $('list'), > > > }; > > > > > > instead? > > > > Done, fixed handling in compiler pass. But I don't think it's a good idea: at > > the point when return object is created DOM is not processed, so $('list') > > returns null. > > > > As long as it's not used before it's assigned externally: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > it's ok, but it could mislead future code readers. > > see below Done. https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:631: list: $('list'), On 2014/09/24 21:38:58, Dan Beam wrote: > either: > > get list() { return $('list'); }, > > or > > document.addEventListener('DOMContentLoaded', function() { > bmm.list = $('list'); > }); Done. https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:16: /** @const */ var localStorage = window.localStorage; On 2014/09/24 21:38:58, Dan Beam wrote: > this is not @const (you're doing localStorage['blah'] = data), how does this > work? does @const do nothing? I would say, @const works this way: it denies any straight reassignments to the name, but does not control key alterings on the object. See: https://gist.github.com/vpavlenko/2884522e910f3728db80 https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:165: if (list.parentId == id) { On 2014/09/24 21:38:58, Dan Beam wrote: > no curlies now Done.
can you upload your newest patchset?
On 2014/09/25 01:42:55, Dan Beam wrote: > can you upload your newest patchset? Sorry about that, done.
https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:92: bmm.list = this; wait, this is already set? https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:171: bmm.tree = this; same https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:631: }, what about: list: /** @type {Element} */(null), // Set when decorated. https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/dnd.js (right): https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/dnd.js:211: return $('list').isSearch() && $('list').contains(overElement); can you use bmm.list instead? https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:163: var list = getRequiredElement('list'); put inside if (opt_callback) https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:353: if (target == $('tree')) { would probably be better as bmm.tree or some cached local reference as well...? guess it's not the end of the world but this is super repetitive. why not var tree = $('tree'); or some other re-use pattern? https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:848: if ($('list').isSearch()) { nit: remove curlies https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:889: * called by forEach), the original parentId of the node will be used. remove "(when called by forEach)"
https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:92: bmm.list = this; On 2014/09/25 04:27:18, Dan Beam wrote: > wait, this is already set? Ok, I reset this assignment. https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right): https://chromiumcodereview.appspot.com/543863002/diff/120001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:171: bmm.tree = this; On 2014/09/25 04:27:18, Dan Beam wrote: > same same https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:631: }, On 2014/09/25 04:27:18, Dan Beam wrote: > what about: > > list: /** @type {Element} */(null), // Set when decorated. Done. https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/dnd.js (right): https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/dnd.js:211: return $('list').isSearch() && $('list').contains(overElement); On 2014/09/25 04:27:18, Dan Beam wrote: > can you use bmm.list instead? Done. https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:163: var list = getRequiredElement('list'); On 2014/09/25 04:27:18, Dan Beam wrote: > put inside if (opt_callback) Done. https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:353: if (target == $('tree')) { On 2014/09/25 04:27:18, Dan Beam wrote: > would probably be better as bmm.tree or some cached local reference as well...? > guess it's not the end of the world but this is super repetitive. why not var > tree = $('tree'); or some other re-use pattern? I replaced $('tree') -> bmm.tree and $('list') -> bmm.list. Do you think it's also worth having local vars caching list/tree in some functions? https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:848: if ($('list').isSearch()) { On 2014/09/25 04:27:18, Dan Beam wrote: > nit: remove curlies Done. https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:889: * called by forEach), the original parentId of the node will be used. On 2014/09/25 04:27:18, Dan Beam wrote: > remove "(when called by forEach)" Done.
lgtm https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://chromiumcodereview.appspot.com/543863002/diff/140001/chrome/browser/r... chrome/browser/resources/bookmark_manager/js/main.js:353: if (target == $('tree')) { On 2014/09/25 15:37:16, Vitaly Pavlenko wrote: > On 2014/09/25 04:27:18, Dan Beam wrote: > > would probably be better as bmm.tree or some cached local reference as > well...? > > guess it's not the end of the world but this is super repetitive. why not var > > tree = $('tree'); or some other re-use pattern? > > I replaced $('tree') -> bmm.tree and $('list') -> bmm.list. Do you think it's > also worth having local vars caching list/tree in some functions? only if it saves lines
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/200001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/200001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/220001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/543863002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as 72eface1103f02bb32aa86b4894f545624aa3fb8
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/5d8e6e26496546a78df69aee799bbe3f60a8ee7f Cr-Commit-Position: refs/heads/master@{#297560} |
