C++ readability change for jochen

C++ readability change for jochen

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67698

[jochen (gone - plz use gerrit), 2010-11-15 15:53:11]

adding brett who was assigned as reviewer. He's on vacation for the next week,
just fyi

[brettw, 2010-11-23 22:44:09]

Overall, this code looks good. I know there are a number of comments here, but
don't let that disturb you - it's our job as readability reviewers to point out
every style concern we can find, no matter what.
                                                                                
Please remember that the purpose of this review is
not to be a commentary on your ability to write code, but
to ensure that you understand Google's style guidelines,
which aren't necessarily better or worse than what you
may be used to, but are almost certainly different and can
take some getting used to.
                                                                                
Note that this isn't a code review: I didn't
thoroughly inspect your code for correctness, that's the job
of the engineers who work in the area it applies to.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:61:
TabContentsToFrameIdMap::value_type(tab_contents, frame_id));
Optional: I might have used std::make_pair(tab_contents, frame_id) here which
would be shorter, although I can see why some people might like your way better.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:97:
tab_contents_map_.equal_range(tab_contents);
This only needs to be indented 4 spaces.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:166: case
NotificationType::TAB_CONTENTS_DESTROYED:
Can you be consistent about whether there's a blank line before each case or
not? I'd say it's not necessary so would just remove the line above this.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:188:
dict->SetString(keys::kUrlKey,
This can fit on one line (same below when you set the url key).

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:235:
dict->SetInteger(keys::kFrameIdKey, navigation_state_.IsMainFrame(frame_id) ?
I always try to format the ternary operators as clearly as possible since they
can be confusing to follow. In this case, I'd probably try to fit it on one line
so move the IsMainFrame call to the next line so you can keep everything
together better.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:246:
NavigationController* controller, long long frame_id) {
the frame_id should probably be on the next line like you do for the other
functions

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:254:
dict->SetInteger(keys::kFrameIdKey, navigation_state_.IsMainFrame(frame_id) ?
Sane ternary formatting suggestion.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webnavigation_api.h (right):

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:26: // be sent for.
This comment and the name of the class don't really mesh in my mine. Does this
state object only exist for error frames (in which case it seems like it should
have some kind of "Error" in the name), or is the tracking everything such that
we can know which ones in particular have errors (in which case the comment
should probably be expanded).

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:33: bool
CanSendEvents(long long frame_id) const;
What is the goal here of using "long long"? I've not seen this used anywhere
else. If you want a 64-bit value, use int64. Same elsewhere.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:77: // Single instance
of the event router.
Function comments should be descriptive, so this should be "Returns the single
instance..." I might say "singleton" instead since that then implies the memory
management of the pointer, where "single instance" might be ambiguous.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:80: void Init();
Who must call this? Normally if I see an init function, I expect to be required
to call it after the constructor, but callers are not in charge of constructing
this object, so this is a bit confusing.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:120: // This method
dispatches events to the extension message service.
"This method dispatches..." -> "Dispatches..."

[jochen (gone - plz use gerrit), 2010-11-24 07:37:36]

The new files are due to changing long long to int64 everywhere

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:61:
TabContentsToFrameIdMap::value_type(tab_contents, frame_id));
On 2010/11/23 22:44:09, brettw wrote:
> Optional: I might have used std::make_pair(tab_contents, frame_id) here which
> would be shorter, although I can see why some people might like your way
better.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:97:
tab_contents_map_.equal_range(tab_contents);
On 2010/11/23 22:44:09, brettw wrote:
> This only needs to be indented 4 spaces.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:166: case
NotificationType::TAB_CONTENTS_DESTROYED:
On 2010/11/23 22:44:09, brettw wrote:
> Can you be consistent about whether there's a blank line before each case or
> not? I'd say it's not necessary so would just remove the line above this.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:188:
dict->SetString(keys::kUrlKey,
On 2010/11/23 22:44:09, brettw wrote:
> This can fit on one line (same below when you set the url key).

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:235:
dict->SetInteger(keys::kFrameIdKey, navigation_state_.IsMainFrame(frame_id) ?
On 2010/11/23 22:44:09, brettw wrote:
> I always try to format the ternary operators as clearly as possible since they
> can be confusing to follow. In this case, I'd probably try to fit it on one
line
> so move the IsMainFrame call to the next line so you can keep everything
> together better.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:246:
NavigationController* controller, long long frame_id) {
On 2010/11/23 22:44:09, brettw wrote:
> the frame_id should probably be on the next line like you do for the other
> functions

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:254:
dict->SetInteger(keys::kFrameIdKey, navigation_state_.IsMainFrame(frame_id) ?
On 2010/11/23 22:44:09, brettw wrote:
> Sane ternary formatting suggestion.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webnavigation_api.h (right):

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:26: // be sent for.
On 2010/11/23 22:44:09, brettw wrote:
> This comment and the name of the class don't really mesh in my mine. Does this
> state object only exist for error frames (in which case it seems like it
should
> have some kind of "Error" in the name), or is the tracking everything such
that
> we can know which ones in particular have errors (in which case the comment
> should probably be expanded).

It's the latter.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:33: bool
CanSendEvents(long long frame_id) const;
On 2010/11/23 22:44:09, brettw wrote:
> What is the goal here of using "long long"? I've not seen this used anywhere
> else. If you want a 64-bit value, use int64. Same elsewhere.

In WebKit, you'd use long long instead. Since that's a value that's being passed
down from WebKit, I've used long long. Seems like in the majority of cases,
we're using int64 instead, so I changed that

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:77: // Single instance
of the event router.
On 2010/11/23 22:44:09, brettw wrote:
> Function comments should be descriptive, so this should be "Returns the single
> instance..." I might say "singleton" instead since that then implies the
memory
> management of the pointer, where "single instance" might be ambiguous.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:80: void Init();
On 2010/11/23 22:44:09, brettw wrote:
> Who must call this? Normally if I see an init function, I expect to be
required
> to call it after the constructor, but callers are not in charge of
constructing
> this object, so this is a bit confusing.

Done.

http://codereview.chromium.org/4997001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.h:120: // This method
dispatches events to the extension message service.
On 2010/11/23 22:44:09, brettw wrote:
> "This method dispatches..." -> "Dispatches..."

Done.

[brettw, 2010-11-29 23:44:37]

LGTM. Please ping me when you've checked this in so I can mark the readability
review bug complete.