Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
293063017:
Updated to read remove all shown when the user searches for a cookie (Closed)
|
Created:
6 years, 7 months ago by radhikabhar Modified:
6 years, 6 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAs the user enters a search string in the cookie search box the remove all button changes to read remove all shown
BUG=363268
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275274
Patch Set 1 #
Total comments: 3
Patch Set 2 : Inserted the lines in cookies_view.js #
Total comments: 3
Patch Set 3 : Updated changes #Patch Set 4 : Updated changes #
Total comments: 1
Patch Set 5 : Updated patch set #Patch Set 6 : #Patch Set 7 : Updated changes #
Total comments: 1
Patch Set 8 : Indentation changes #
Messages
Total messages: 30 (0 generated)
drive-by review https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/cookies_view.js (left): https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/cookies_view.js:110: did you intentionally delete these lines? https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/cookies_view.js (right): https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/cookies_view.js:93: document.querySelector('.remove-all-cookies-button').innerHTML = Instead of using innerHTML, can you use .value?
Hey Radhika, here are some tips on using the code review tool: - The "description" should be a single, static description of what this CL does. Can you please update it to describe the patch? - If you want to respond to comments, do it by going to the comment itself and then responding. - If you want to reply in general, at the bottom click "reply" and then "send message" instead of updating the description. If you use the normal reply feature, it will send me an email so I know to look at the CL.
https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/cookies_view.js (right): https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/cookies_view.js:93: document.querySelector('.remove-all-cookies-button').innerHTML = On 2014/05/23 17:18:12, felt wrote: > Instead of using innerHTML, can you use .value? Radhika's response was: > .Value does not change the button text that's why innerHTML OK, I see. This explains why: http://permadi.com/tutorial/jsInnerHTMLDOM/.
On 2014/05/23 22:40:14, felt wrote: > https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... > File chrome/browser/resources/options/cookies_view.js (right): > > https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... > chrome/browser/resources/options/cookies_view.js:93: > document.querySelector('.remove-all-cookies-button').innerHTML = > On 2014/05/23 17:18:12, felt wrote: > > Instead of using innerHTML, can you use .value? > > Radhika's response was: > > .Value does not change the button text that's why innerHTML > > OK, I see. This explains why: http://permadi.com/tutorial/jsInnerHTMLDOM/. I updated the Description and removed the .trim() function
On 2014/05/24 00:06:33, radhikabhar wrote: > On 2014/05/23 22:40:14, felt wrote: > > > https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... > > File chrome/browser/resources/options/cookies_view.js (right): > > > > > https://codereview.chromium.org/293063017/diff/1/chrome/browser/resources/opt... > > chrome/browser/resources/options/cookies_view.js:93: > > document.querySelector('.remove-all-cookies-button').innerHTML = > > On 2014/05/23 17:18:12, felt wrote: > > > Instead of using innerHTML, can you use .value? > > > > Radhika's response was: > > > .Value does not change the button text that's why innerHTML > > > > OK, I see. This explains why: http://permadi.com/tutorial/jsInnerHTMLDOM/. > > I updated the Description and removed the .trim() function Updated the description
hi dan, can you please review this CL for radhika? (note: this is her first CL.) cheers, adrienne
looks pretty good https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/app/genera... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/app/genera... chrome/app/generated_resources.grd:10479: <message name="IDS_COOKIES_REMOVE_ALL_SHOWN_LABEL" desc="The label of the 'Remove All Shown' button after a query search string is entered in the Cookies Window"> "entered in" -> "entered in" (double space -> single space) https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... File chrome/browser/resources/options/cookies_view.js (right): https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... chrome/browser/resources/options/cookies_view.js:99: } i think you can shave some lines with this: var stringId = document.querySelector('.cookies-search-box').value ? 'remove_all_shown_cookie' : 'remove_all_cookie'; document.querySelector('.remove-all-cookies-button').innerHTML = loadTimeData.getString(stringId);
On 2014/05/24 02:03:52, Dan Beam wrote: > looks pretty good > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/app/genera... > File chrome/app/generated_resources.grd (right): > Removed the extra space and shaved off lines > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/app/genera... > chrome/app/generated_resources.grd:10479: <message > name="IDS_COOKIES_REMOVE_ALL_SHOWN_LABEL" desc="The label of the 'Remove All > Shown' button after a query search string is entered in the Cookies Window"> > "entered in" -> "entered in" (double space -> single space) > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... > File chrome/browser/resources/options/cookies_view.js (right): > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... > chrome/browser/resources/options/cookies_view.js:99: } > i think you can shave some lines with this: > > var stringId = document.querySelector('.cookies-search-box').value ? > 'remove_all_shown_cookie' : 'remove_all_cookie'; > document.querySelector('.remove-all-cookies-button').innerHTML = > loadTimeData.getString(stringId);
On 2014/05/27 16:35:33, radhikabhar wrote: > On 2014/05/24 02:03:52, Dan Beam wrote: > > looks pretty good > > > > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/app/genera... > > File chrome/app/generated_resources.grd (right): > > Removed the extra space and shaved off lines > > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/app/genera... > > chrome/app/generated_resources.grd:10479: <message > > name="IDS_COOKIES_REMOVE_ALL_SHOWN_LABEL" desc="The label of the 'Remove All > > Shown' button after a query search string is entered in the Cookies Window"> > > "entered in" -> "entered in" (double space -> single space) > > > > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... > > File chrome/browser/resources/options/cookies_view.js (right): > > > > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... > > chrome/browser/resources/options/cookies_view.js:99: } > > i think you can shave some lines with this: > > > > var stringId = document.querySelector('.cookies-search-box').value ? > > 'remove_all_shown_cookie' : 'remove_all_cookie'; > > document.querySelector('.remove-all-cookies-button').innerHTML = > > loadTimeData.getString(stringId); Updated Changes
https://codereview.chromium.org/293063017/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/cookies_view.js (right): https://codereview.chromium.org/293063017/diff/60001/chrome/browser/resources... chrome/browser/resources/options/cookies_view.js:98: } i don't see the updated changes here. can you upload again?
On 2014/05/27 17:44:29, Dan Beam wrote: > https://codereview.chromium.org/293063017/diff/60001/chrome/browser/resources... > File chrome/browser/resources/options/cookies_view.js (right): > > https://codereview.chromium.org/293063017/diff/60001/chrome/browser/resources... > chrome/browser/resources/options/cookies_view.js:98: } > i don't see the updated changes here. can you upload again? The changes should be visible now
https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... File chrome/browser/resources/options/cookies_view.js (right): https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... chrome/browser/resources/options/cookies_view.js:99: } On 2014/05/24 02:03:52, Dan Beam wrote: > i think you can shave some lines with this: > > var stringId = document.querySelector('.cookies-search-box').value ? > 'remove_all_shown_cookie' : 'remove_all_cookie'; > document.querySelector('.remove-all-cookies-button').innerHTML = > loadTimeData.getString(stringId); what happened to re-writing the code like this? ^
On 2014/05/27 21:04:31, Dan Beam wrote: > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... > File chrome/browser/resources/options/cookies_view.js (right): > > https://chromiumcodereview.appspot.com/293063017/diff/20001/chrome/browser/re... > chrome/browser/resources/options/cookies_view.js:99: } > On 2014/05/24 02:03:52, Dan Beam wrote: > > i think you can shave some lines with this: > > > > var stringId = document.querySelector('.cookies-search-box').value ? > > 'remove_all_shown_cookie' : 'remove_all_cookie'; > > document.querySelector('.remove-all-cookies-button').innerHTML = > > loadTimeData.getString(stringId); > > what happened to re-writing the code like this? ^ Updated the changes
lgtm w/nit https://chromiumcodereview.appspot.com/293063017/diff/120001/chrome/browser/r... File chrome/browser/resources/options/cookies_view.js (right): https://chromiumcodereview.appspot.com/293063017/diff/120001/chrome/browser/r... chrome/browser/resources/options/cookies_view.js:92: 'remove_all_shown_cookie' : 'remove_all_cookie'; add 2 more spaces for line 92 and line 94 (we use a 4 space indent for continuations)
On 2014/05/27 22:19:11, Dan Beam wrote: > lgtm w/nit > > https://chromiumcodereview.appspot.com/293063017/diff/120001/chrome/browser/r... > File chrome/browser/resources/options/cookies_view.js (right): > > https://chromiumcodereview.appspot.com/293063017/diff/120001/chrome/browser/r... > chrome/browser/resources/options/cookies_view.js:92: 'remove_all_shown_cookie' : > 'remove_all_cookie'; > add 2 more spaces for line 92 and line 94 (we use a 4 space indent for > continuations) Added spaces
On 2014/05/27 22:42:31, radhikabhar wrote: > On 2014/05/27 22:19:11, Dan Beam wrote: > > lgtm w/nit > > > > > https://chromiumcodereview.appspot.com/293063017/diff/120001/chrome/browser/r... > > File chrome/browser/resources/options/cookies_view.js (right): > > > > > https://chromiumcodereview.appspot.com/293063017/diff/120001/chrome/browser/r... > > chrome/browser/resources/options/cookies_view.js:92: 'remove_all_shown_cookie' > : > > 'remove_all_cookie'; > > add 2 more spaces for line 92 and line 94 (we use a 4 space indent for > > continuations) > > Added spaces Radhika, I noticed that this code never actually landed, but it looks like it works and you have approval from Dan. What you have to do now is click the checkbox next to "Commit" so that the code will go into the commit queue.
The CQ bit was checked by radhikabhar@chromium.org
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/293063017/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was checked by felt@chromium.org
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/293063017/140001
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
Message was sent while issue was closed.
Change committed as 275274 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
