Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1378073002:
Removed NOTIFY_ERROR() macro from GpuVEAH. (Closed)
|
Created:
5 years, 2 months ago by msu.koo Modified:
5 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoved NOTIFY_ERROR() macro from GpuVEAH.
NOTIFY_ERROR() will not work as expected if used under
non-braced if-else.
This patch removed this macro and replace them into
original implementations.
BUG=535705
Committed: https://crrev.com/12c3e22a0f8300e969e598c63d19f7ec627a9212
Cr-Commit-Position: refs/heads/master@{#353223}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comment reflected #
Total comments: 4
Patch Set 3 : Comments Reflected #
Total comments: 3
Patch Set 4 : comment reflected #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 27 (7 generated)
msu.koo@samsung.com changed reviewers: + mcasas@chromium.org, posciak@chromium.org
Comments about [2] for Posciak was reflected on this patch. PTAL :)
https://codereview.chromium.org/1378073002/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://codereview.chromium.org/1378073002/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:114: DLOG(ERROR) << "EncodeSharedMemory(): cannot " We could probably fit these messages in 2 lines now? https://codereview.chromium.org/1378073002/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:164: Please remove this empty line.
Thank you for your review. Your comments are all addressed. PTAL :) https://codereview.chromium.org/1378073002/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://codereview.chromium.org/1378073002/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:114: DLOG(ERROR) << "EncodeSharedMemory(): cannot " On 2015/09/30 08:15:15, Pawel Osciak wrote: > We could probably fit these messages in 2 lines now? Done. https://codereview.chromium.org/1378073002/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:164: On 2015/09/30 08:15:15, Pawel Osciak wrote: > Please remove this empty line. Done.
https://codereview.chromium.org/1378073002/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://codereview.chromium.org/1378073002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:125: DLOG(ERROR) << "EncodeSharedMemory(): failed to " Sorry, but this could also fit in 2 lines. https://codereview.chromium.org/1378073002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:164: << "UseOutputBitstreamBuffer(): failed to duplicate buffer handle " And this as well.
Also addressed :) https://codereview.chromium.org/1378073002/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://codereview.chromium.org/1378073002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:125: DLOG(ERROR) << "EncodeSharedMemory(): failed to " On 2015/09/30 08:22:20, Pawel Osciak wrote: > Sorry, but this could also fit in 2 lines. Done. https://codereview.chromium.org/1378073002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:164: << "UseOutputBitstreamBuffer(): failed to duplicate buffer handle " On 2015/09/30 08:22:20, Pawel Osciak wrote: > And this as well. Done.
Tip: To avoid having to revisit the style guide there's the command `git cl format`, that'd run clang-format on the diffs and make it chromium. clang-format can be added to editors e.g. sublime, eclipse and emacs. https://codereview.chromium.org/1378073002/diff/40001/content/common/gpu/clie... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://codereview.chromium.org/1378073002/diff/40001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:212: PostNotifyError(kPlatformFailureError); It's unfortunate that PostNotifyError(bla) is also doing a DLOG(ERROR) << bla [1] Maybe we could extend the method to do PostNotifyError(Error error, const std::string& message) and DLOG(ERROR) in that method. (Hint: in this case, base::StringPrintf() is your friend to compose error strings). [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
Thank your for your comments and tip! it really helps me :) Also left some of my thought upon yours. https://codereview.chromium.org/1378073002/diff/40001/content/common/gpu/clie... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://codereview.chromium.org/1378073002/diff/40001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:212: PostNotifyError(kPlatformFailureError); On 2015/09/30 15:32:07, mcasas wrote: > It's unfortunate that PostNotifyError(bla) is also doing a > DLOG(ERROR) << bla > [1] > > Maybe we could extend the method to do > PostNotifyError(Error error, const std::string& message) > > and DLOG(ERROR) in that method. (Hint: in this case, > base::StringPrintf() is your friend to compose error strings). > > > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... DLOG comes with line number and file name. If we consolidate DLOG into PostNotifyError, the line number of log will always indicate PostNotifyError function. I think it would be better to notify the real error position by leaving the DLOG as they are. WDYT?
https://chromiumcodereview.appspot.com/1378073002/diff/40001/content/common/g... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://chromiumcodereview.appspot.com/1378073002/diff/40001/content/common/g... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:212: PostNotifyError(kPlatformFailureError); On 2015/10/01 04:22:13, msu.koo wrote: > On 2015/09/30 15:32:07, mcasas wrote: > > It's unfortunate that PostNotifyError(bla) is also doing a > > DLOG(ERROR) << bla > > [1] > > > > Maybe we could extend the method to do > > PostNotifyError(Error error, const std::string& message) > > > > and DLOG(ERROR) in that method. (Hint: in this case, > > base::StringPrintf() is your friend to compose error strings). > > > > > > > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > DLOG comes with line number and file name. If we consolidate DLOG into > PostNotifyError, the line number of log will always indicate PostNotifyError > function. I think it would be better to notify the real error position by > leaving the DLOG as they are. WDYT? > Yeah leaving the DLOG(ERROR) sounds logical, but then what's the use of a DVLOG(2) inside PostnotifyError()? What about using class Location [1] and using it as PostNotifyError() parameter? You can use then in the call: PostNotifyError(FROM_HERE, kBigErrorEnum) and then inside PostNotifyError() print out a nice DLOG(ERROR), instead of a DVLOG(2). [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/location.h&sq...
On 2015/10/05 16:18:48, mcasas wrote: > https://chromiumcodereview.appspot.com/1378073002/diff/40001/content/common/g... > File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): > > https://chromiumcodereview.appspot.com/1378073002/diff/40001/content/common/g... > content/common/gpu/client/gpu_video_encode_accelerator_host.cc:212: > PostNotifyError(kPlatformFailureError); > On 2015/10/01 04:22:13, msu.koo wrote: > > On 2015/09/30 15:32:07, mcasas wrote: > > > It's unfortunate that PostNotifyError(bla) is also doing a > > > DLOG(ERROR) << bla > > > [1] > > > > > > Maybe we could extend the method to do > > > PostNotifyError(Error error, const std::string& message) > > > > > > and DLOG(ERROR) in that method. (Hint: in this case, > > > base::StringPrintf() is your friend to compose error strings). > > > > > > > > > > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > DLOG comes with line number and file name. If we consolidate DLOG into > > PostNotifyError, the line number of log will always indicate PostNotifyError > > function. I think it would be better to notify the real error position by > > leaving the DLOG as they are. WDYT? > > > > Yeah leaving the DLOG(ERROR) sounds logical, > but then what's the use of a DVLOG(2) inside > PostnotifyError()? > > What about using class Location [1] and using it > as PostNotifyError() parameter? You can use then in > the call: > PostNotifyError(FROM_HERE, kBigErrorEnum) > > and then inside PostNotifyError() print out a > nice DLOG(ERROR), instead of a DVLOG(2). > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/location.h&sq... Thank you for your comment. I tried to pass Location to PostNotifyError to print the log, but it requires another LOG macro because LOG() does not get the location as arguments. How about just removing DVLOG from PostNotifyError, because all PostNotifyError always comes with DLOG() in this CL. (,which means DVLOG in PostNotifyError is a duplicated log.)
On 2015/10/06 02:28:25, msu.koo wrote: > On 2015/10/05 16:18:48, mcasas wrote: > > > https://chromiumcodereview.appspot.com/1378073002/diff/40001/content/common/g... > > File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): > > > > > https://chromiumcodereview.appspot.com/1378073002/diff/40001/content/common/g... > > content/common/gpu/client/gpu_video_encode_accelerator_host.cc:212: > > PostNotifyError(kPlatformFailureError); > > On 2015/10/01 04:22:13, msu.koo wrote: > > > On 2015/09/30 15:32:07, mcasas wrote: > > > > It's unfortunate that PostNotifyError(bla) is also doing a > > > > DLOG(ERROR) << bla > > > > [1] > > > > > > > > Maybe we could extend the method to do > > > > PostNotifyError(Error error, const std::string& message) > > > > > > > > and DLOG(ERROR) in that method. (Hint: in this case, > > > > base::StringPrintf() is your friend to compose error strings). > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > > > DLOG comes with line number and file name. If we consolidate DLOG into > > > PostNotifyError, the line number of log will always indicate PostNotifyError > > > function. I think it would be better to notify the real error position by > > > leaving the DLOG as they are. WDYT? > > > > > > > Yeah leaving the DLOG(ERROR) sounds logical, > > but then what's the use of a DVLOG(2) inside > > PostnotifyError()? > > > > What about using class Location [1] and using it > > as PostNotifyError() parameter? You can use then in > > the call: > > PostNotifyError(FROM_HERE, kBigErrorEnum) > > > > and then inside PostNotifyError() print out a > > nice DLOG(ERROR), instead of a DVLOG(2). > > > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/location.h&sq... > > Thank you for your comment. > I tried to pass Location to PostNotifyError to print the log, but it requires > another LOG macro because LOG() does not get the location as arguments. > How about just removing DVLOG from PostNotifyError, because all PostNotifyError > always comes with DLOG() in this CL. > (,which means DVLOG in PostNotifyError is a duplicated log.) I feel like leaving to the developers of the future to have the discipline to always bunde together a DVLOG() + PostNotifyError is an unnecessary burden. What about this void PostNotifyError(const Location& location, Error error) { // DCHECK() thread DLOG(ERROR) << "Error from " << location.function_name() << " ( " << location.file_name() << ":" << location.line_number() << ") " << error; // Launch the error }
@mcasas, your comments are reflected. PTAL :)
LGTM % nits below https://codereview.chromium.org/1378073002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right): https://codereview.chromium.org/1378073002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:206: << location.line_number() << ")" space after "("? https://codereview.chromium.org/1378073002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_video_encode_accelerator_host.cc:207: << message << "(error = " << error << ")"; space before "(" ?
The CQ bit was checked by msu.koo@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1378073002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1378073002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1378073002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
msu.koo@samsung.com changed reviewers: + kbr@chromium.org
PTAL :)
kbr@chromium.org changed reviewers: + ccameron@chromium.org
+ccameron. I won't be able to review this today.
lgtm
The CQ bit was checked by msu.koo@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1378073002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1378073002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/12c3e22a0f8300e969e598c63d19f7ec627a9212 Cr-Commit-Position: refs/heads/master@{#353223} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
