Bug 46710 – [Chromium] Linking error due to wrong guard in WebIDBTransaction.h

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andrei Popescu]

[Chromium] Linking error due to wrong guard in WebIDBTransaction.h

[Andrei Popescu]

Created attachment 69034 [details]
Patch

[Jeremy Orlow]

Comment on attachment 69034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review

r=me

> WebKit/chromium/public/WebIDBTransaction.h:65
> +        // FIXME: this is never called in the renderer. Find a cleaner solution.

Comments that apply to the whole function really seem like they should go right above the function not in them.  Is there other precedent for something like this?  If not, can you please stop doing this?

Also, try not to make the comments Chromium architecture specific.  Maybe just say that this is only meant to be called from within WebCore?

[Jeremy Orlow]

+fishd cause this is a webkit patch

[Andrei Popescu]

(In reply to comment #2)
> (From update of attachment 69034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review
> 
> r=me
> 
> > WebKit/chromium/public/WebIDBTransaction.h:65
> > +        // FIXME: this is never called in the renderer. Find a cleaner solution.
> 
> Comments that apply to the whole function really seem like they should go right above the function not in them.  Is there other precedent for something like this?  If not, can you please stop doing this?
> 

OK ok :)

> Also, try not to make the comments Chromium architecture specific.  Maybe just say that this is only meant to be called from within WebCore?

Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong?

[Jeremy Orlow]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 69034 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review
> > 
> > r=me
> > 
> > > WebKit/chromium/public/WebIDBTransaction.h:65
> > > +        // FIXME: this is never called in the renderer. Find a cleaner solution.
> > 
> > Comments that apply to the whole function really seem like they should go right above the function not in them.  Is there other precedent for something like this?  If not, can you please stop doing this?
> > 
> 
> OK ok :)
> 
> > Also, try not to make the comments Chromium architecture specific.  Maybe just say that this is only meant to be called from within WebCore?
> 
> Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong?

WebKit code should not depend on Chromium code.  Even if this code is designed to be consumed by Chromium.

Plus Darin said so.  :-)

[Andrei Popescu]

Committed r68504: <http://trac.webkit.org/changeset/68504>

[Andrei Popescu]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 69034 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review
> > > 
> > > r=me
> > > 
> > > > WebKit/chromium/public/WebIDBTransaction.h:65
> > > > +        // FIXME: this is never called in the renderer. Find a cleaner solution.
> > > 
> > > Comments that apply to the whole function really seem like they should go right above the function not in them.  Is there other precedent for something like this?  If not, can you please stop doing this?
> > > 
> > 
> > OK ok :)
> > 
> > > Also, try not to make the comments Chromium architecture specific.  Maybe just say that this is only meant to be called from within WebCore?
> > 
> > Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong?
> 
> WebKit code should not depend on Chromium code.  Even if this code is designed to be consumed by Chromium.
>

Yes, that comment did depend on Chromium code...fair enough, changed.

[Jeremy Orlow]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 69034 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review
> > 
> > r=me
> > 
> > > WebKit/chromium/public/WebIDBTransaction.h:65
> > > +        // FIXME: this is never called in the renderer. Find a cleaner solution.
> > 
> > Comments that apply to the whole function really seem like they should go right above the function not in them.  Is there other precedent for something like this?  If not, can you please stop doing this?
> > 
> 
> OK ok :)
> 
> > Also, try not to make the comments Chromium architecture specific.  Maybe just say that this is only meant to be called from within WebCore?
> 
> Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong?

WebKit code should not depend on Chromium code.  Even if this code is designed to be consumed by Chromium.

Plus Darin said so.  :-)

[Jeremy Orlow]

(In reply to comment #8)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 69034 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review
> > > 
> > > r=me
> > > 
> > > > WebKit/chromium/public/WebIDBTransaction.h:65
> > > > +        // FIXME: this is never called in the renderer. Find a cleaner solution.
> > > 
> > > Comments that apply to the whole function really seem like they should go right above the function not in them.  Is there other precedent for something like this?  If not, can you please stop doing this?
> > > 
> > 
> > OK ok :)
> > 
> > > Also, try not to make the comments Chromium architecture specific.  Maybe just say that this is only meant to be called from within WebCore?
> > 
> > Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong?
> 
> WebKit code should not depend on Chromium code.  Even if this code is designed to be consumed by Chromium.
> 
> Plus Darin said so.  :-)

Ugh...stupid bugzilla...sorry.