Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581983002:
Fix Incorrect date/time info under chrome://crashes. (Closed)
DescriptionFix Incorrect date/time info under chrome://crashes.
This cl patches in the solution devised by phistuck@gmail.com. See bug 674249 for detail.
BUG=674249
Patch Set 1 #Patch Set 2 : Change the placeholder of upload time. #Patch Set 3 : Patch in more changes suggested by phistuck@gmail.com. #
Total comments: 2
Patch Set 4 : Split crash+upload into two separate strings. #
Total comments: 1
Messages
Total messages: 56 (17 generated)
The CQ bit was checked by jiameng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix Incorrect date/time info under chrome://crashes. This cl patches in the solution devised by phistuck@gmail.com. See bug 674249 for detail. BUG=674249 ========== to ========== Fix Incorrect date/time info under chrome://crashes. This cl patches in the solution devised by phistuck@gmail.com. See bug 674249 for detail. BUG=674249 ==========
jiameng@chromium.org changed reviewers: + phistuck@gmail.com
Hello, I've patched in the change you've suggested. Please review. Thanks, Jia
That looks good! Thank you! Can you just test it first?
On 2016/12/16 11:24:35, PhistucK wrote: > That looks good! Thank you! > > Can you just test it first? From crbug.com/674249 - Re #11: I believe the issue is that both of the placeholders in the IDS_CRASH_CRASH_TIME_FORMAT template are specified as $1, i.e. to substitute the first of the supplied parameters, which is crash-time. Can you also replace the second one with $2? Thank you!
The CQ bit was checked by jiameng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/16 21:38:39, PhistucK wrote: > On 2016/12/16 11:24:35, PhistucK wrote: > > That looks good! Thank you! > > > > Can you just test it first? > From crbug.com/674249 - > Re #11: I believe the issue is that both of the placeholders in the > IDS_CRASH_CRASH_TIME_FORMAT template are specified as $1, i.e. to substitute the > first of the supplied parameters, which is crash-time. > > Can you also replace the second one with $2? > > Thank you! I've changed the second one with $2 as suggested. What is the best way to test it out? I'm not in the office now, so I'm not able to build a dev version and launch a chrome browser. Thanks, Jia
On 2016/12/16 22:17:44, Jia wrote: > On 2016/12/16 21:38:39, PhistucK wrote: > > On 2016/12/16 11:24:35, PhistucK wrote: > > > That looks good! Thank you! > > > > > > Can you just test it first? > > From crbug.com/674249 - > > Re #11: I believe the issue is that both of the placeholders in the > > IDS_CRASH_CRASH_TIME_FORMAT template are specified as $1, i.e. to substitute > the > > first of the supplied parameters, which is crash-time. > > > > Can you also replace the second one with $2? > > > > Thank you! > > I've changed the second one with $2 as suggested. What is the best way to test > it out? I'm not in the office now, so I'm not able to build a dev version and > launch a chrome browser. > Thanks, > Jia If you know how to change PAK files (for example, C:\Program Files (x86)\google\Chrome\Application\55.0.2883.87\Locales\en.pak), you might be able to change the string there, I guess and test it. Otherwise, I think you must build Chrome (not Chromium) in order to test crash report uploading. :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/16 22:21:43, PhistucK wrote: > On 2016/12/16 22:17:44, Jia wrote: > > On 2016/12/16 21:38:39, PhistucK wrote: > > > On 2016/12/16 11:24:35, PhistucK wrote: > > > > That looks good! Thank you! > > > > > > > > Can you just test it first? > > > From crbug.com/674249 - > > > Re #11: I believe the issue is that both of the placeholders in the > > > IDS_CRASH_CRASH_TIME_FORMAT template are specified as $1, i.e. to substitute > > the > > > first of the supplied parameters, which is crash-time. > > > > > > Can you also replace the second one with $2? > > > > > > Thank you! > > > > I've changed the second one with $2 as suggested. What is the best way to test > > it out? I'm not in the office now, so I'm not able to build a dev version and > > launch a chrome browser. > > Thanks, > > Jia > > If you know how to change PAK files (for example, C:\Program Files > (x86)\google\Chrome\Application\55.0.2883.87\Locales\en.pak), you might be able > to change the string there, I guess and test it. Otherwise, I think you must > build Chrome (not Chromium) in order to test crash report uploading. :( I built Chrome but I have no crash reports (no crashes for a while!). I haven't found any way to force a crash, any suggestions? Alternatively, what do you think if I submit this cl and put in more fixes if the problem remains? Thanks, Jia
On 2016/12/18 23:12:44, Jia wrote: > On 2016/12/16 22:21:43, PhistucK wrote: > > On 2016/12/16 22:17:44, Jia wrote: > > > On 2016/12/16 21:38:39, PhistucK wrote: > > > > On 2016/12/16 11:24:35, PhistucK wrote: > > > > > That looks good! Thank you! > > > > > > > > > > Can you just test it first? > > > > From crbug.com/674249 - > > > > Re #11: I believe the issue is that both of the placeholders in the > > > > IDS_CRASH_CRASH_TIME_FORMAT template are specified as $1, i.e. to > substitute > > > the > > > > first of the supplied parameters, which is crash-time. > > > > > > > > Can you also replace the second one with $2? > > > > > > > > Thank you! > > > > > > I've changed the second one with $2 as suggested. What is the best way to > test > > > it out? I'm not in the office now, so I'm not able to build a dev version > and > > > launch a chrome browser. > > > Thanks, > > > Jia > > > > If you know how to change PAK files (for example, C:\Program Files > > (x86)\google\Chrome\Application\55.0.2883.87\Locales\en.pak), you might be > able > > to change the string there, I guess and test it. Otherwise, I think you must > > build Chrome (not Chromium) in order to test crash report uploading. :( > > I built Chrome but I have no crash reports (no crashes for a while!). I haven't > found any way to force a crash, any suggestions? Alternatively, what do you > think if I submit this cl and put in more fixes if the problem remains? > Thanks, > Jia You can use chrome://crash for inducing a crash. I think we might two strings - both of the original and the changed one. In the issue - https://bugs.chromium.org/p/chromium/issues/detail?id=674249#c12 - they says that one of the crash reporting systems capture the crash time stamp and one does not, so we need to be prepared for no crash time stamp (as it used to be) as well. I will try and cook up the change later today (unless you beat me to it ;)).
I am trying to figure out how to detect that capture_time does not exist (the previous code relied on whether the crash report was uploaded or not instead of checking whether the value exists). I am not proficient in C++ - is capture_time.isnull() valid when no one initializes capture_time (which is base::Time) with a value? https://cs.chromium.org/chromium/src/components/upload_list/upload_list.cc?sq... simply does not set a value in those cases. Once I figure that out, writing the change will be easy.
On 2016/12/20 07:37:46, PhistucK wrote: > I am trying to figure out how to detect that capture_time does not exist (the > previous code relied on whether the crash report was uploaded or not instead of > checking whether the value exists). > I am not proficient in C++ - is capture_time.isnull() valid when no one > initializes capture_time (which is base::Time) with a value? > https://cs.chromium.org/chromium/src/components/upload_list/upload_list.cc?sq... > simply does not set a value in those cases. > Once I figure that out, writing the change will be easy. The C++ in crashes_ui_util.cc currently formats the time values into |upload_time| and |time| fields in the crash description, and |upload_time| is currently optional. The Javascript in crashes.js then takes those and applies them as parameters to the format string. As discussed on the bug, we have two implementations, and each crash can be either uploaded, or not-uploaded-yet, so four possible states: 1. No crash-time and no upload-time (Breakpad crashes that are not yet uploaded). 2. Crash-time but no upload-time (Crashpad crashes not yet uploaded). 3. Crash-time and upload-time (Crashpad uploaded crashes). 4. Upload-time, but no crash-time (Breakpad crashes that were uploaded). My suggestion would be to: 1. Change crashes_ui_utils.cc to have both upload_time and time be optional fields. 2. Split the crash message format into two separate format strings in the GRDP - one for "Crash captured at <time>" and one for "Crash uploaded at <time>". 3. Update crashes.js, and the corresponding HTML, to allow for both strings, and set each one only if the corresponding time field is actually set. (Note that you can test #2 & #3 locally by just hacking the crashes_ui_utils.cc to set (or not set) the fields, with dummy values. :)
On 2016/12/20 19:19:55, Wez wrote: > On 2016/12/20 07:37:46, PhistucK wrote: > > I am trying to figure out how to detect that capture_time does not exist (the > > previous code relied on whether the crash report was uploaded or not instead > of > > checking whether the value exists). > > I am not proficient in C++ - is capture_time.isnull() valid when no one > > initializes capture_time (which is base::Time) with a value? > > > https://cs.chromium.org/chromium/src/components/upload_list/upload_list.cc?sq... > > simply does not set a value in those cases. > > Once I figure that out, writing the change will be easy. > > The C++ in crashes_ui_util.cc currently formats the time values into > |upload_time| and |time| fields in the crash description, and |upload_time| is > currently optional. The Javascript in crashes.js then takes those and applies > them as parameters to the format string. > > As discussed on the bug, we have two implementations, and each crash can be > either uploaded, or not-uploaded-yet, so four possible states: > > 1. No crash-time and no upload-time (Breakpad crashes that are not yet > uploaded). > 2. Crash-time but no upload-time (Crashpad crashes not yet uploaded). > 3. Crash-time and upload-time (Crashpad uploaded crashes). > 4. Upload-time, but no crash-time (Breakpad crashes that were uploaded). > > My suggestion would be to: > 1. Change crashes_ui_utils.cc to have both upload_time and time be optional > fields. > 2. Split the crash message format into two separate format strings in the GRDP - > one for "Crash captured at <time>" and one for "Crash uploaded at <time>". > 3. Update crashes.js, and the corresponding HTML, to allow for both strings, and > set each one only if the corresponding time field is actually set. > > (Note that you can test #2 & #3 locally by just hacking the crashes_ui_utils.cc > to set (or not set) the fields, with dummy values. :) Yes, that was my intention and both of them are currently (or were before my change) optional - but how do I detect that capture_time is not populated? Will info.capture_time.is_null() throw? (I am asking, because I am a JavaScript developer and in JavaScript, it would, because it is an uninitialized value, but maybe in C++ it does not)
I tried http://webcompiler.cloudapp.net/ using this small code - #include <string> #include <iostream> using namespace std; class FindMax { public: string maxNum() { string strMy("hello world"); return strMy; } }; int main (int) { FindMax f; cout << f.maxNum() << endl; } And it compiled, ran and the output was indeed "hello world", so I think I understand what I have to do. Here is the change - crash_strings.grdp - - <message name="IDS_CRASH_CRASH_TIME_FORMAT" desc="Format for crash entry occurrence time on chrome://crashes"> - Crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph>, reported on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph> - </message> + <message name="IDS_CRASH_CRASH_TIME_FORMAT" desc="Format for crash entry upload time on chrome://crashes"> + Automatically reported <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph> + </message> + <message name="IDS_CRASH_CRASH_AND_UPLOAD_TIME_FORMAT" desc="Format for crash entry occurrence time and its upload time on chrome://crashes"> + Crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph>, reported on <ph name="UPLOAD_TIME">$2<ex>Tuesday, January 25, 2011 2:58:02 PM</ex></ph> + </message> crashes_ui_util.cc - {"crashTimeFormat", IDS_CRASH_CRASH_TIME_FORMAT}, + {"crashAndUploadTimeFormat", IDS_CRASH_CRASH_AND_UPLOAD_TIME_FORMAT}, }; // ... + if (!info.capture_time.is_null()) { - crash->SetString("time", - base::TimeFormatFriendlyDateAndTime(info.capture_time)); + crash->SetString("time", + base::TimeFormatFriendlyDateAndTime(info.capture_time)); + } crashes.js - + if (!crash.time) { - date.textContent = loadTimeData.getStringF('crashTimeFormat', - crash.time, - crash.upload_time); + date.textContent = loadTimeData.getStringF('crashTimeFormat', + crash.upload_time); + } else { + date.textContent = loadTimeData.getStringF('crashAndUploadTimeFormat', + crash.time, + crash.upload_time); + } Does that look fine? Just in case Rietveld garbled the patch, I also attached it to the original issue - https://bugs.chromium.org/p/chromium/issues/detail?id=664430#c11 Please, try this change... Thank you very much and sorry for the trouble!
The CQ bit was checked by jiameng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/20 20:44:13, PhistucK wrote: > I tried http://webcompiler.cloudapp.net/ using this small code - > #include <string> > #include <iostream> > > using namespace std; > > > class FindMax > { > > public: > > string maxNum() { > string strMy("hello world"); > return strMy; > } > }; > > > int main (int) { > FindMax f; > cout << f.maxNum() << endl; > } > > > And it compiled, ran and the output was indeed "hello world", so I think I > understand what I have to do. > > Here is the change - > crash_strings.grdp - > - <message name="IDS_CRASH_CRASH_TIME_FORMAT" desc="Format for crash entry > occurrence time on chrome://crashes"> > - Crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, > 2011 2:58:02 PM</ex></ph>, reported on <ph name="CRASH_TIME">$1<ex>Tuesday, > January 25, 2011 2:58:02 PM</ex></ph> > - </message> > + <message name="IDS_CRASH_CRASH_TIME_FORMAT" desc="Format for crash entry > upload time on chrome://crashes"> > + Automatically reported <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, > 2011 2:58:02 PM</ex></ph> > + </message> > + <message name="IDS_CRASH_CRASH_AND_UPLOAD_TIME_FORMAT" desc="Format for > crash entry occurrence time and its upload time on chrome://crashes"> > + Crash report captured on <ph name="CRASH_TIME">$1<ex>Tuesday, January 25, > 2011 2:58:02 PM</ex></ph>, reported on <ph name="UPLOAD_TIME">$2<ex>Tuesday, > January 25, 2011 2:58:02 PM</ex></ph> > + </message> > > crashes_ui_util.cc - > {"crashTimeFormat", IDS_CRASH_CRASH_TIME_FORMAT}, > + {"crashAndUploadTimeFormat", IDS_CRASH_CRASH_AND_UPLOAD_TIME_FORMAT}, > }; > > // ... > > + if (!info.capture_time.is_null()) { > - crash->SetString("time", > - base::TimeFormatFriendlyDateAndTime(info.capture_time)); > + crash->SetString("time", > + > base::TimeFormatFriendlyDateAndTime(info.capture_time)); > + } > > crashes.js - > + if (!crash.time) { > - date.textContent = loadTimeData.getStringF('crashTimeFormat', > - crash.time, > - crash.upload_time); > + date.textContent = loadTimeData.getStringF('crashTimeFormat', > + crash.upload_time); > + } else { > + date.textContent = > loadTimeData.getStringF('crashAndUploadTimeFormat', > + crash.time, > + crash.upload_time); > + } > > > Does that look fine? > > Just in case Rietveld garbled the patch, I also attached it to the original > issue - > https://bugs.chromium.org/p/chromium/issues/detail?id=664430#c11 > > Please, try this change... > > Thank you very much and sorry for the trouble! I've put in your patch, please take a look, thanks! Jia
That looks right, thank you!
On 2016/12/21 16:28:24, PhistucK wrote: > That looks right, thank you!
The CQ bit was checked by jiameng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Would you please LGTM the cl? Thanks! Jia
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... File components/crash/core/browser/resources/crashes.js (right): https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... components/crash/core/browser/resources/crashes.js:68: crash.upload_time); My suggestion was actually to have two *separate* strings for the two times, e.g. of the form "Captured at <time>." and "Reported at <time>.". That way if you have a crash.time set then you can display "Captured at ..." even for crashes that are not yet uploaded. Basically we'd just move this date-string logic outside the if (uploaded), with something like: var date = document.createElement('p'); date.textContent = "" if (crash.time) { date.textContent += loadTimeData.getStringF('crashTimeFormat', crash.time); } if (crash.upload_time) { date.textContent += loadTimeData.getStringF('crashUploadTimeFormat', crash.upload_time); } crashBlock.appendChild(date); if (uploaded) { ... } Or something along those lines. :)
On 2016/12/22 03:03:13, Wez wrote: > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > File components/crash/core/browser/resources/crashes.js (right): > > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > components/crash/core/browser/resources/crashes.js:68: crash.upload_time); > My suggestion was actually to have two *separate* strings for the two times, > e.g. of the form "Captured at <time>." and "Reported at <time>.". > > That way if you have a crash.time set then you can display "Captured at ..." > even for crashes that are not yet uploaded. Basically we'd just move this > date-string logic outside the if (uploaded), with something like: > > var date = document.createElement('p'); > date.textContent = "" > if (crash.time) { > date.textContent += loadTimeData.getStringF('crashTimeFormat', crash.time); > } > if (crash.upload_time) { > date.textContent += loadTimeData.getStringF('crashUploadTimeFormat', > crash.upload_time); > } > crashBlock.appendChild(date); > > if (uploaded) { > ... > } > > Or something along those lines. :) Thanks! I've changed it. Also, I suppose I don't need to change crash_strings.grdp? Thanks, Jia
Thanks! I've made the changes, PTAL. Also, I guess crash_strings.grdp doesn't need to be changed? Thanks, Jia https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... File components/crash/core/browser/resources/crashes.js (right): https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... components/crash/core/browser/resources/crashes.js:68: crash.upload_time); On 2016/12/22 03:03:13, Wez wrote: > My suggestion was actually to have two *separate* strings for the two times, > e.g. of the form "Captured at <time>." and "Reported at <time>.". > > That way if you have a crash.time set then you can display "Captured at ..." > even for crashes that are not yet uploaded. Basically we'd just move this > date-string logic outside the if (uploaded), with something like: > > var date = document.createElement('p'); > date.textContent = "" > if (crash.time) { > date.textContent += loadTimeData.getStringF('crashTimeFormat', crash.time); > } > if (crash.upload_time) { > date.textContent += loadTimeData.getStringF('crashUploadTimeFormat', > crash.upload_time); > } > crashBlock.appendChild(date); > > if (uploaded) { > ... > } > > Or something along those lines. :) Done.
On 2016/12/22 04:16:55, Jia wrote: > Thanks! I've made the changes, PTAL. > Also, I guess crash_strings.grdp doesn't need to be changed? > Thanks, > Jia > > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > File components/crash/core/browser/resources/crashes.js (right): > > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > components/crash/core/browser/resources/crashes.js:68: crash.upload_time); > On 2016/12/22 03:03:13, Wez wrote: > > My suggestion was actually to have two *separate* strings for the two times, > > e.g. of the form "Captured at <time>." and "Reported at <time>.". > > > > That way if you have a crash.time set then you can display "Captured at ..." > > even for crashes that are not yet uploaded. Basically we'd just move this > > date-string logic outside the if (uploaded), with something like: > > > > var date = document.createElement('p'); > > date.textContent = "" > > if (crash.time) { > > date.textContent += loadTimeData.getStringF('crashTimeFormat', crash.time); > > } > > if (crash.upload_time) { > > date.textContent += loadTimeData.getStringF('crashUploadTimeFormat', > > crash.upload_time); > > } > > crashBlock.appendChild(date); > > > > if (uploaded) { > > ... > > } > > > > Or something along those lines. :) > > Done. Actually, I think I deliberately did not do that - I think this gives more room for localization to phrase the sentence in a more natural way. Wez, do you still think this is a better way to go? We already have a similar sentence (for only capture time) when it is not uploaded. What does it gain?
On 2016/12/22 05:27:06, PhistucK wrote: > On 2016/12/22 04:16:55, Jia wrote: > > Thanks! I've made the changes, PTAL. > > Also, I guess crash_strings.grdp doesn't need to be changed? > > Thanks, > > Jia > > > > > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > > File components/crash/core/browser/resources/crashes.js (right): > > > > > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > > components/crash/core/browser/resources/crashes.js:68: crash.upload_time); > > On 2016/12/22 03:03:13, Wez wrote: > > > My suggestion was actually to have two *separate* strings for the two times, > > > e.g. of the form "Captured at <time>." and "Reported at <time>.". > > > > > > That way if you have a crash.time set then you can display "Captured at ..." > > > even for crashes that are not yet uploaded. Basically we'd just move this > > > date-string logic outside the if (uploaded), with something like: > > > > > > var date = document.createElement('p'); > > > date.textContent = "" > > > if (crash.time) { > > > date.textContent += loadTimeData.getStringF('crashTimeFormat', > crash.time); > > > } > > > if (crash.upload_time) { > > > date.textContent += loadTimeData.getStringF('crashUploadTimeFormat', > > > crash.upload_time); > > > } > > > crashBlock.appendChild(date); > > > > > > if (uploaded) { > > > ... > > > } > > > > > > Or something along those lines. :) > > > > Done. > > Actually, I think I deliberately did not do that - I think this gives more room > for localization to phrase the sentence in a more natural way. > Wez, do you still think this is a better way to go? > We already have a similar sentence (for only capture time) when it is not > uploaded. > What does it gain? It would allow us to list a crash in chrome://crashes even before it has been uploaded, with the time at which the crash was reported listed, if available. The difference in translation quality is a fair point, but this is a page that (hopefully) few users ever see, so so long as the translations make sense, I'm not so worried about how natural they sound - but that's just my opinion. :) Otherwise you can't actually list the crash-time for the crash until it has uploaded, even if that information is available (i.e. under Crashpad), which seems unfortunate. You can support that with both applied to a single format string if you have three distinct format strings (crash-only, upload-only, upload+crash), which seems overkill, though of course once everything runs Crashpad we could remove the upload-only case. WDYT?
On 2016/12/22 19:34:14, Wez wrote: > On 2016/12/22 05:27:06, PhistucK wrote: > > On 2016/12/22 04:16:55, Jia wrote: > > > Thanks! I've made the changes, PTAL. > > > Also, I guess crash_strings.grdp doesn't need to be changed? > > > Thanks, > > > Jia > > > > > > > > > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > > > File components/crash/core/browser/resources/crashes.js (right): > > > > > > > > > https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... > > > components/crash/core/browser/resources/crashes.js:68: crash.upload_time); > > > On 2016/12/22 03:03:13, Wez wrote: > > > > My suggestion was actually to have two *separate* strings for the two > times, > > > > e.g. of the form "Captured at <time>." and "Reported at <time>.". > > > > > > > > That way if you have a crash.time set then you can display "Captured at > ..." > > > > even for crashes that are not yet uploaded. Basically we'd just move this > > > > date-string logic outside the if (uploaded), with something like: > > > > > > > > var date = document.createElement('p'); > > > > date.textContent = "" > > > > if (crash.time) { > > > > date.textContent += loadTimeData.getStringF('crashTimeFormat', > > crash.time); > > > > } > > > > if (crash.upload_time) { > > > > date.textContent += loadTimeData.getStringF('crashUploadTimeFormat', > > > > crash.upload_time); > > > > } > > > > crashBlock.appendChild(date); > > > > > > > > if (uploaded) { > > > > ... > > > > } > > > > > > > > Or something along those lines. :) > > > > > > Done. > > > > Actually, I think I deliberately did not do that - I think this gives more > room > > for localization to phrase the sentence in a more natural way. > > Wez, do you still think this is a better way to go? > > We already have a similar sentence (for only capture time) when it is not > > uploaded. > > What does it gain? > > It would allow us to list a crash in chrome://crashes even before it has been > uploaded, with the time at which the crash was reported listed, if available. > The difference in translation quality is a fair point, but this is a page that > (hopefully) few users ever see, so so long as the translations make sense, I'm > not so worried about how natural they sound - but that's just my opinion. :) > > Otherwise you can't actually list the crash-time for the crash until it has > uploaded, even if that information is available (i.e. under Crashpad), which > seems unfortunate. You can support that with both applied to a single format > string if you have three distinct format strings (crash-only, upload-only, > upload+crash), which seems overkill, though of course once everything runs > Crashpad we could remove the upload-only case. > > WDYT? Hm, why can I not list the crash time until it has uploaded? I did not touch that part. The JavaScript code I changed is within the "if (uploaded)" branch. It it were not uploaded, the existing flow would be chosen, where the crash time is shown and an option to upload the crash report is given. Line 111 and below on the left side of https://codereview.chromium.org/2581983002/diff2/40001:60001/components/crash... Actually, the new code (patch set 4) does not seem right, it was pulled out of the "if (uploaded)" branch. This would display almost-duplicate lines.
On 22 December 2016 at 11:40, <phistuck@gmail.com> wrote: [snip] > On 2016/12/22 19:34:14, Wez wrote: > > It would allow us to list a crash in chrome://crashes even before it has > been > > uploaded, with the time at which the crash was reported listed, if > available. > > The difference in translation quality is a fair point, but this is a > page that > > (hopefully) few users ever see, so so long as the translations make > sense, I'm > > not so worried about how natural they sound - but that's just my > opinion. :) > > > > Otherwise you can't actually list the crash-time for the crash until it > has > > uploaded, even if that information is available (i.e. under Crashpad), > which > > seems unfortunate. You can support that with both applied to a single > format > > string if you have three distinct format strings (crash-only, > upload-only, > > upload+crash), which seems overkill, though of course once everything > runs > > Crashpad we could remove the upload-only case. > > > > WDYT? > > Hm, why can I not list the crash time until it has uploaded? I did not > touch > that part. > In patch set #3 you have string formats for upload_time-only, and for crash_time+upload_time - so there's no way to display only the crash_time, if the crash has not yet uploaded. The JavaScript code I changed is within the "if (uploaded)" branch. It it > were > not uploaded, the existing flow would be chosen, where the crash time is > shown > and an option to upload the crash report is given. > Oh, I hadn't realised that each case was explicitly adding crash.time printout, so yes, you only need to cope with the two cases here. You will need to fix those other uses of crash.time not to add printout if crash.time is empty, to match what crashes_ui_util.cc is doing. > Line 111 and below on the left side of > https://codereview.chromium.org/2581983002/diff2/40001: > 60001/components/crash/core/browser/resources/crashes.js > > Actually, the new code (patch set 4) does not seem right, it was pulled > out of > the "if (uploaded)" branch. This would display almost-duplicate lines. > > https://codereview.chromium.org/2581983002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/22 19:54:03, Wez wrote: > On 22 December 2016 at 11:40, <mailto:phistuck@gmail.com> wrote: > [snip] > > > > On 2016/12/22 19:34:14, Wez wrote: > > > It would allow us to list a crash in chrome://crashes even before it has > > been > > > uploaded, with the time at which the crash was reported listed, if > > available. > > > The difference in translation quality is a fair point, but this is a > > page that > > > (hopefully) few users ever see, so so long as the translations make > > sense, I'm > > > not so worried about how natural they sound - but that's just my > > opinion. :) > > > > > > Otherwise you can't actually list the crash-time for the crash until it > > has > > > uploaded, even if that information is available (i.e. under Crashpad), > > which > > > seems unfortunate. You can support that with both applied to a single > > format > > > string if you have three distinct format strings (crash-only, > > upload-only, > > > upload+crash), which seems overkill, though of course once everything > > runs > > > Crashpad we could remove the upload-only case. > > > > > > WDYT? > > > > Hm, why can I not list the crash time until it has uploaded? I did not > > touch > > that part. > > > > In patch set #3 you have string formats for upload_time-only, and for > crash_time+upload_time - so there's no way to display only the crash_time, > if the crash has not yet uploaded. > > The JavaScript code I changed is within the "if (uploaded)" branch. It it > > were > > not uploaded, the existing flow would be chosen, where the crash time is > > shown > > and an option to upload the crash report is given. > > > > Oh, I hadn't realised that each case was explicitly adding crash.time > printout, so yes, you only need to cope with the two cases here. > > You will need to fix those other uses of crash.time not to add printout if > crash.time is empty, to match what crashes_ui_util.cc is doing. > > Why do I? That null check is really only accommodating post-upload scenarios. I believe pre-upload scenarios had crash times. The original code - did not have any null check, it only relied on the state - uploaded or not uploaded. I think you might be missing some context. This change fixes the new logic that assumed that we do have both of the times (crash and upload) by the time we get to the upload state. Here is the original code - https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... Both of the C++ and JavaScript codes had the same check that decided whether upload time or crash time (and their respective state text) is used - https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... So, previously, before it is uploaded, chrome://crashes always showed the crash time, after it is uploaded, chrome://crashes always showed the upload time. My previous change was to always show the crash time (and upload time if it is uploaded). Since Breakpad removes the crash time after uploading (if I understand correctly), I simply had to accommodate the no-crash-time-after-upload scenario until Crashpad comes along. Are we on the same page (patch set 3) or do you still think I should accommodate those two scenarios? Or do you perhaps want me to add a comment above the is_null I added? Something like - // Breakpad does not have crash time after a report is uploaded, // but it does have it before a report is uploaded. And maybe one more in the JavaScript - // No need to accommodate for no crash time and not-yet-uploaded // scenario, as the crash time is always available before a report // is uploaded, even in Breakpad. I hope I did not confuse you further...
A friendly ping, Wez. :)
On 2016/12/22 22:18:18, PhistucK wrote: > On 2016/12/22 19:54:03, Wez wrote: > > On 22 December 2016 at 11:40, <mailto:phistuck@gmail.com> wrote: > > [snip] > > > > > > > On 2016/12/22 19:34:14, Wez wrote: > > > > It would allow us to list a crash in chrome://crashes even before it has > > > been > > > > uploaded, with the time at which the crash was reported listed, if > > > available. > > > > The difference in translation quality is a fair point, but this is a > > > page that > > > > (hopefully) few users ever see, so so long as the translations make > > > sense, I'm > > > > not so worried about how natural they sound - but that's just my > > > opinion. :) > > > > > > > > Otherwise you can't actually list the crash-time for the crash until it > > > has > > > > uploaded, even if that information is available (i.e. under Crashpad), > > > which > > > > seems unfortunate. You can support that with both applied to a single > > > format > > > > string if you have three distinct format strings (crash-only, > > > upload-only, > > > > upload+crash), which seems overkill, though of course once everything > > > runs > > > > Crashpad we could remove the upload-only case. > > > > > > > > WDYT? > > > > > > Hm, why can I not list the crash time until it has uploaded? I did not > > > touch > > > that part. > > > > > > > In patch set #3 you have string formats for upload_time-only, and for > > crash_time+upload_time - so there's no way to display only the crash_time, > > if the crash has not yet uploaded. > > > > The JavaScript code I changed is within the "if (uploaded)" branch. It it > > > were > > > not uploaded, the existing flow would be chosen, where the crash time is > > > shown > > > and an option to upload the crash report is given. > > > > > > > Oh, I hadn't realised that each case was explicitly adding crash.time > > printout, so yes, you only need to cope with the two cases here. > > > > You will need to fix those other uses of crash.time not to add printout if > > crash.time is empty, to match what crashes_ui_util.cc is doing. > > > > > Why do I? Because the change in crashes_ui_utils.cc (see https://codereview.chromium.org/2581983002/diff/40001/components/crash/core/b... lines 76-79) mean that crash.time is not set if the capture_time was not available (i.e. for Linux & ChromeOS, which still use Breakpad). > That null check is really only accommodating post-upload scenarios. I believe > pre-upload scenarios had crash times. The original code - did not have any null > check, it only relied on the state - uploaded or not uploaded. No, if you look at the original state of crashes_ui_util.cc (both before and after https://codereview.chromium.org/2568953003/diff/1/components/crash/core/brows...), that code always set a crash.time (though its value will often be bogus under Breakpad). Since we have updated crashes_ui_util.cc to only set a crash.time value if capture_time is non-null, we need to also update the JS to cope with it not being set (e.g. by simply skipping adding a summary line for crash.time). > I think you might be missing some context. This change fixes the new logic that > assumed that we do have both of the times (crash and upload) by the time we get > to the upload state. > Here is the original code - > https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... > Both of the C++ and JavaScript codes had the same check that decided whether > upload time or crash time (and their respective state text) is used - > https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... > https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... > https://chromium.googlesource.com/chromium/src/+/cc7d54eab860b6c17661ae641a60... > > So, previously, before it is uploaded, chrome://crashes always showed the crash > time, after it is uploaded, chrome://crashes always showed the upload time. My > previous change was to always show the crash time (and upload time if it is > uploaded). Since Breakpad removes the crash time after uploading (if I > understand correctly), I simply had to accommodate the > no-crash-time-after-upload scenario until Crashpad comes along. The problem is that Breakpad never records a capture_time at all, only Crashpad does, so if we are having each entry display both crash time and upload time then we need to cope with both upload time being null (i.e. not-yet-uploaded) and with crash time being null (i.e. using Breakpad). > Are we on the same page (patch set 3) or do you still think I should accommodate > those two scenarios? > Or do you perhaps want me to add a comment above the is_null I added? Something > like - > // Breakpad does not have crash time after a report is uploaded, > // but it does have it before a report is uploaded. > And maybe one more in the JavaScript - > // No need to accommodate for no crash time and not-yet-uploaded > // scenario, as the crash time is always available before a report > // is uploaded, even in Breakpad. > > > I hope I did not confuse you further... My recommendation for this CL is to go with Patch-set #3 but remove the crashTime/crash.time handling from the non-uploaded case in the JS - that should mean we only get the crash and/or upload times displayed once. However, see the additional comments regarding the formatting of the |date| element text.
https://codereview.chromium.org/2581983002/diff/60001/components/crash/core/b... File components/crash/core/browser/resources/crashes.js (right): https://codereview.chromium.org/2581983002/diff/60001/components/crash/core/b... components/crash/core/browser/resources/crashes.js:67: date.textContent += loadTimeData.getStringF('crashUploadTimeFormat', Note that the format strings don't include any punctation, so in the case of both crash.time and crash.upload_time being set, we need to either include punctuation in the format strings, or add it here. As phistuck@gmail previously suggested, we can do more natural translations if we have a single format string, so the other alternative would be to have *three* format strings (crash-only, upload-only, crash+upload), and update the JS etc accordingly.
I will comment (or make changes, after reading everything one more time) more thoroughly tomorrow, but I just wanted to ask a question regarding your comment - > No, if you look at the original state of crashes_ui_util.cc (both before and > after > https://codereview.chromium.org/2568953003/diff/1/components/crash/core/brows...), > that code always set a crash.time (though its value will often be bogus under > Breakpad). So does that mean that the original code (before any changes of mine were made) was also wrong and displayed incorrect information?
On 2016/12/27 21:53:02, PhistucK wrote: > I will comment (or make changes, after reading everything one more time) more > thoroughly tomorrow, but I just wanted to ask a question regarding your comment > - > > > No, if you look at the original state of crashes_ui_util.cc (both before and > > after > > > https://codereview.chromium.org/2568953003/diff/1/components/crash/core/brows...), > > that code always set a crash.time (though its value will often be bogus under > > Breakpad). > > So does that mean that the original code (before any changes of mine were made) > was also wrong and displayed incorrect information? No, I think the original code was correct because some other part of chrome://crashes avoids showing not-yet-uploaded crashes in the list under Breakpad platforms. Your earlier CL breaks things under Breakpad by assuming that crash-time will be set, which is never the case with Breakpad. If you like, I can submit a revert of your earlier CL, and we can finish this up and verify that it copes correctly with uploaded, pre-uploaded, Crashpad and Breakpad, and then land that. WDYT?
On 2016/12/27 22:18:24, Wez wrote: > On 2016/12/27 21:53:02, PhistucK wrote: > > I will comment (or make changes, after reading everything one more time) more > > thoroughly tomorrow, but I just wanted to ask a question regarding your > comment > > - > > > > > No, if you look at the original state of crashes_ui_util.cc (both before and > > > after > > > > > > https://codereview.chromium.org/2568953003/diff/1/components/crash/core/brows...), > > > that code always set a crash.time (though its value will often be bogus > under > > > Breakpad). > > > > So does that mean that the original code (before any changes of mine were > made) > > was also wrong and displayed incorrect information? > > No, I think the original code was correct because some other part of > chrome://crashes avoids showing not-yet-uploaded crashes in the list under > Breakpad platforms. > > Your earlier CL breaks things under Breakpad by assuming that crash-time will be > set, which is never the case with Breakpad. > > If you like, I can submit a revert of your earlier CL, and we can finish this up > and verify that it copes correctly with uploaded, pre-uploaded, Crashpad and > Breakpad, and then land that. WDYT? That might be good, because I have not seen anything that does what you just wrote (but it might be in some other file). Revert, I guess and I will come back to it in the coming days, after you let me know if it works well with Breakpad somehow... Thank you!
Yeah, I'm wondering if Breakpad itself actually only reports the uploaded crashes to chrome://crashes, while Crashpad reports both. Will prep a CL to revert the earlier change, for now. On 27 December 2016 at 22:20, <phistuck@gmail.com> wrote: > On 2016/12/27 22:18:24, Wez wrote: > > On 2016/12/27 21:53:02, PhistucK wrote: > > > I will comment (or make changes, after reading everything one more > time) > more > > > thoroughly tomorrow, but I just wanted to ask a question regarding your > > comment > > > - > > > > > > > No, if you look at the original state of crashes_ui_util.cc (both > before > and > > > > after > > > > > > > > > > https://codereview.chromium.org/2568953003/diff/1/ > components/crash/core/brows...), > > > > that code always set a crash.time (though its value will often be > bogus > > under > > > > Breakpad). > > > > > > So does that mean that the original code (before any changes of mine > were > > made) > > > was also wrong and displayed incorrect information? > > > > No, I think the original code was correct because some other part of > > chrome://crashes avoids showing not-yet-uploaded crashes in the list > under > > Breakpad platforms. > > > > Your earlier CL breaks things under Breakpad by assuming that crash-time > will > be > > set, which is never the case with Breakpad. > > > > If you like, I can submit a revert of your earlier CL, and we can finish > this > up > > and verify that it copes correctly with uploaded, pre-uploaded, Crashpad > and > > Breakpad, and then land that. WDYT? > > That might be good, because I have not seen anything that does what you > just > wrote (but it might be in some other file). Revert, I guess and I will > come back > to it in the coming days, after you let me know if it works well with > Breakpad > somehow... > Thank you! > > https://codereview.chromium.org/2581983002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/27 22:23:42, Wez wrote: > Yeah, I'm wondering if Breakpad itself actually only reports the uploaded > crashes to chrome://crashes, while Crashpad reports both. > > Will prep a CL to revert the earlier change, for now. > > On 27 December 2016 at 22:20, <mailto:phistuck@gmail.com> wrote: > > > On 2016/12/27 22:18:24, Wez wrote: > > > On 2016/12/27 21:53:02, PhistucK wrote: > > > > I will comment (or make changes, after reading everything one more > > time) > > more > > > > thoroughly tomorrow, but I just wanted to ask a question regarding your > > > comment > > > > - > > > > > > > > > No, if you look at the original state of crashes_ui_util.cc (both > > before > > and > > > > > after > > > > > > > > > > > > > > https://codereview.chromium.org/2568953003/diff/1/ > > components/crash/core/brows...), > > > > > that code always set a crash.time (though its value will often be > > bogus > > > under > > > > > Breakpad). > > > > > > > > So does that mean that the original code (before any changes of mine > > were > > > made) > > > > was also wrong and displayed incorrect information? > > > > > > No, I think the original code was correct because some other part of > > > chrome://crashes avoids showing not-yet-uploaded crashes in the list > > under > > > Breakpad platforms. > > > > > > Your earlier CL breaks things under Breakpad by assuming that crash-time > > will > > be > > > set, which is never the case with Breakpad. > > > > > > If you like, I can submit a revert of your earlier CL, and we can finish > > this > > up > > > and verify that it copes correctly with uploaded, pre-uploaded, Crashpad > > and > > > Breakpad, and then land that. WDYT? > > > > That might be good, because I have not seen anything that does what you > > just > > wrote (but it might be in some other file). Revert, I guess and I will > > come back > > to it in the coming days, after you let me know if it works well with > > Breakpad > > somehow... > > Thank you! > > > > https://codereview.chromium.org/2581983002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. As you're preparing another cl, should I drop this? Thanks, Jia
On 2016/12/28 01:35:15, Jia wrote: > On 2016/12/27 22:23:42, Wez wrote: > > Yeah, I'm wondering if Breakpad itself actually only reports the uploaded > > crashes to chrome://crashes, while Crashpad reports both. > > > > Will prep a CL to revert the earlier change, for now. > > > > On 27 December 2016 at 22:20, <mailto:phistuck@gmail.com> wrote: > > > > > On 2016/12/27 22:18:24, Wez wrote: > > > > On 2016/12/27 21:53:02, PhistucK wrote: > > > > > I will comment (or make changes, after reading everything one more > > > time) > > > more > > > > > thoroughly tomorrow, but I just wanted to ask a question regarding your > > > > comment > > > > > - > > > > > > > > > > > No, if you look at the original state of crashes_ui_util.cc (both > > > before > > > and > > > > > > after > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2568953003/diff/1/ > > > components/crash/core/brows...), > > > > > > that code always set a crash.time (though its value will often be > > > bogus > > > > under > > > > > > Breakpad). > > > > > > > > > > So does that mean that the original code (before any changes of mine > > > were > > > > made) > > > > > was also wrong and displayed incorrect information? > > > > > > > > No, I think the original code was correct because some other part of > > > > chrome://crashes avoids showing not-yet-uploaded crashes in the list > > > under > > > > Breakpad platforms. > > > > > > > > Your earlier CL breaks things under Breakpad by assuming that crash-time > > > will > > > be > > > > set, which is never the case with Breakpad. > > > > > > > > If you like, I can submit a revert of your earlier CL, and we can finish > > > this > > > up > > > > and verify that it copes correctly with uploaded, pre-uploaded, Crashpad > > > and > > > > Breakpad, and then land that. WDYT? > > > > > > That might be good, because I have not seen anything that does what you > > > just > > > wrote (but it might be in some other file). Revert, I guess and I will > > > come back > > > to it in the coming days, after you let me know if it works well with > > > Breakpad > > > somehow... > > > Thank you! > > > > > > https://codereview.chromium.org/2581983002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > As you're preparing another cl, should I drop this? > Thanks, > Jia This can be dropped, I might revisit it with a new patch on top of the old original code in the coming days.
On 2016/12/28 06:39:56, PhistucK wrote: > On 2016/12/28 01:35:15, Jia wrote: > > On 2016/12/27 22:23:42, Wez wrote: > > > Yeah, I'm wondering if Breakpad itself actually only reports the uploaded > > > crashes to chrome://crashes, while Crashpad reports both. > > > > > > Will prep a CL to revert the earlier change, for now. > > > > > > On 27 December 2016 at 22:20, <mailto:phistuck@gmail.com> wrote: > > > > > > > On 2016/12/27 22:18:24, Wez wrote: > > > > > On 2016/12/27 21:53:02, PhistucK wrote: > > > > > > I will comment (or make changes, after reading everything one more > > > > time) > > > > more > > > > > > thoroughly tomorrow, but I just wanted to ask a question regarding > your > > > > > comment > > > > > > - > > > > > > > > > > > > > No, if you look at the original state of crashes_ui_util.cc (both > > > > before > > > > and > > > > > > > after > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2568953003/diff/1/ > > > > components/crash/core/brows...), > > > > > > > that code always set a crash.time (though its value will often be > > > > bogus > > > > > under > > > > > > > Breakpad). > > > > > > > > > > > > So does that mean that the original code (before any changes of mine > > > > were > > > > > made) > > > > > > was also wrong and displayed incorrect information? > > > > > > > > > > No, I think the original code was correct because some other part of > > > > > chrome://crashes avoids showing not-yet-uploaded crashes in the list > > > > under > > > > > Breakpad platforms. > > > > > > > > > > Your earlier CL breaks things under Breakpad by assuming that crash-time > > > > will > > > > be > > > > > set, which is never the case with Breakpad. > > > > > > > > > > If you like, I can submit a revert of your earlier CL, and we can finish > > > > this > > > > up > > > > > and verify that it copes correctly with uploaded, pre-uploaded, Crashpad > > > > and > > > > > Breakpad, and then land that. WDYT? > > > > > > > > That might be good, because I have not seen anything that does what you > > > > just > > > > wrote (but it might be in some other file). Revert, I guess and I will > > > > come back > > > > to it in the coming days, after you let me know if it works well with > > > > Breakpad > > > > somehow... > > > > Thank you! > > > > > > > > https://codereview.chromium.org/2581983002/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > As you're preparing another cl, should I drop this? > > Thanks, > > Jia > > This can be dropped, I might revisit it with a new patch on top of the old > original code in the coming days. I'll drop it then, thanks for your help! :)
Message was sent while issue was closed.
FYI: Closing since we have the new layout landed - thanks for working on this!
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2622363006/ by jiameng@chromium.org. The reason for reverting is: Drop this cl because new layout was set up in a separately cl..
Message was sent while issue was closed.
This CL never landed, so no need to revert, surely? On 12 Jan 2017 2:18 pm, <jiameng@chromium.org> wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2622363006/ by jiameng@chromium.org. > > The reason for reverting is: Drop this cl because new layout was set up in > a > separately cl.. > > https://codereview.chromium.org/2581983002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I've closed it, thanks! Jia On Fri, Jan 13, 2017 at 9:23 AM, Wez <wez@chromium.org> wrote: > This CL never landed, so no need to revert, surely? > > On 12 Jan 2017 2:18 pm, <jiameng@chromium.org> wrote: > >> A revert of this CL (patchset #4 id:60001) has been created in >> https://codereview.chromium.org/2622363006/ by jiameng@chromium.org. >> >> The reason for reverting is: Drop this cl because new layout was set up >> in a >> separately cl.. >> >> https://codereview.chromium.org/2581983002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
