Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8726005:
Fix an incorrect comment in the Java Bridge when calling array methods (Closed)
|
Created:
9 years ago by Steve Block Modified:
9 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, dpranke-watch+content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix an incorrect comment in the Java Bridge when calling array methods
BUG=105547
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112204
Patch Set 1 #Patch Set 2 : Fixed comment #
Total comments: 3
Patch Set 3 : Clarifies existing comments #Messages
Total messages: 14 (0 generated)
LGTM, but meta-comment :) http://codereview.chromium.org/8726005/diff/1002/content/browser/renderer_hos... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8726005/diff/1002/content/browser/renderer_hos... content/browser/renderer_host/java/java_bound_object.cc:157: // maintain existing behavior. We should call the method and convert the is the second sentence a TODO? also, I got comments that we should avoid we :) so this either would be TODO(steveblock) call the method and.. or something like "The new behavior should call.."
http://codereview.chromium.org/8726005/diff/1002/content/browser/renderer_hos... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8726005/diff/1002/content/browser/renderer_hos... content/browser/renderer_host/java/java_bound_object.cc:157: // maintain existing behavior. We should call the method and convert the It't not really a TODO. This file has many comments of this style. They're used to document areas where we don't follow the LiveConnect spec. The first sentence is what we do. The second is what we should do to match the spec. We have no immediate plans to match the spec, but may revisit this in the future. If using 'we' is discouraged, I could update all the comments to use something like 'The spec requires calling the method ...'. Can you point me to the guidance on 'we'?
http://codereview.chromium.org/8726005/diff/1002/content/browser/renderer_hos... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8726005/diff/1002/content/browser/renderer_hos... content/browser/renderer_host/java/java_bound_object.cc:157: // maintain existing behavior. We should call the method and convert the On 2011/11/29 11:18:00, Steve Block wrote: > It't not really a TODO. This file has many comments of this style. They're used > to document areas where we don't follow the LiveConnect spec. The first sentence > is what we do. The second is what we should do to match the spec. We have no > immediate plans to match the spec, but may revisit this in the future. got it, thanks for the clarification! > > If using 'we' is discouraged, I could update all the comments to use something > like 'The spec requires calling the method ...'. Can you point me to the > guidance on 'we'? here's the comment I had: http://codereview.chromium.org/8586054/diff/2001/base/base.gypi#newcode606 yes, I think the "The spec requires" would make it clearer the distinction between LIVECONNECT_COMPLIANCE and the spec.. Maybe something like: LIVECONNECT_COMPLIANCE: Existing behavior, %s. The spec requires %s.
Updated existing comments as suggested.
LGTM (not an owner..)
Adding avi for OWNERS approval
The comments seem more like TODOs. Can you link to the bug in the comments?
Never mind. I see the comment up top now. LGTM
These aren't really TODOs. The comments are to document cases where we differ from the LiveConnect spec. We don't have any current plans to fix these. See the comment about LIVECONNECT_COMPLIANCE at the top of the file. I could file a bug just to track this if you'd like?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/steveblock@chromium.org/8726005/6001
Try job failure for 8726005-6001 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/steveblock@chromium.org/8726005/6001
Change committed as 112204 |
