Bug 54361 – [Chromium] Issue 50685: Move CC icons to chrome/common and implement loadPlatformResource in RendererWebKitClientImpl.

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Naoki Takano]

[Chromium] Issue 50685:	Move CC icons to chrome/common and implement loadPlatformResource in RendererWebKitClientImpl.

[Naoki Takano]

Created attachment 82271 [details]
Patch

[Naoki Takano]

Could you review?

As I already mentioned in chrome CL, we have to commit at once.

But the build itself file without chrome side commit. Just not working correctly.

[James Hawkins]

+darin since I can't technically review.

I'm not sure why you're adding loadPlatformResource() when there's already a loadResource() which does the same thing.

When I said use loadPlatformResource() in the TODO, I mean Image::loadPlatformResource().

[Darin Fisher (:fishd, Google)]

Comment on attachment 82271 [details]
Patch

R- based on jhawkins feedback

[Naoki Takano]

Thank you for your review!!

[Naoki Takano]

Image::loadPlatformResource() calls PlatformBridge::loadPlatformImageResource(name),
And in loadPlatformImageResource(name), webKitClient()->loadResource(name) is called for now.

Also you mentioned "implement loadPlatformResource in RendererWebKitClientImpl" as a subject in http://code.google.com/p/chromium/issues/detail?id=50685

That's why I added loadPlatformResource() in WebKitClient which is the super class of RendererWebKitClientImpl.

Or do I have to change the call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of PlatformBridge::loadPlatformImageResource(name)?

What do you think?

[Ilya Sherman]

(In reply to comment #6)
> Image::loadPlatformResource() calls PlatformBridge::loadPlatformImageResource(name),
> And in loadPlatformImageResource(name), webKitClient()->loadResource(name) is called for now.
> 
> Also you mentioned "implement loadPlatformResource in RendererWebKitClientImpl" as a subject in http://code.google.com/p/chromium/issues/detail?id=50685
> 
> That's why I added loadPlatformResource() in WebKitClient which is the super class of RendererWebKitClientImpl.
> 
> Or do I have to change the call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of PlatformBridge::loadPlatformImageResource(name)?
> 
> What do you think?

Let's see if we can just use Image::loadPlatformResource() for now, unless there's reason to go beyond that.

[Naoki Takano]

Ilya,

Thank you for your comment.

I will change to call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of calling PlatformBridge::loadPlatformImageResource(name).

Thanks,

(In reply to comment #7)
> (In reply to comment #6)
> > Image::loadPlatformResource() calls PlatformBridge::loadPlatformImageResource(name),
> > And in loadPlatformImageResource(name), webKitClient()->loadResource(name) is called for now.
> > 
> > Also you mentioned "implement loadPlatformResource in RendererWebKitClientImpl" as a subject in http://code.google.com/p/chromium/issues/detail?id=50685
> > 
> > That's why I added loadPlatformResource() in WebKitClient which is the super class of RendererWebKitClientImpl.
> > 
> > Or do I have to change the call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of PlatformBridge::loadPlatformImageResource(name)?
> > 
> > What do you think?
> 
> Let's see if we can just use Image::loadPlatformResource() for now, unless there's reason to go beyond that.

[Ilya Sherman]

(In reply to comment #8)
> I will change to call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of calling PlatformBridge::loadPlatformImageResource(name).

What's wrong with calling PlatformBridge::loadPlatformImageResource()?

[Naoki Takano]

IMHO, calling PlatformBridge::loadPlatformImageResource() is better.

Because the function is also used from Source/WebCore/platform/graphics/chromium/ImageChromiumMac.mm
And if we move it, we have to move ImageChromiumMac.mm also.

That is another reason I changed PlatformBridge::loadPlatformImageResource().

(In reply to comment #9)
> (In reply to comment #8)
> > I will change to call RendererWebKitClientImpl::loadPlatformResource() directly from Image::loadPlatformResource() instead of calling PlatformBridge::loadPlatformImageResource(name).
> 
> What's wrong with calling PlatformBridge::loadPlatformImageResource()?

[Ilya Sherman]

To be clearer: Do we really need WebKit changes in order to move the resources in Chromium?  If so, why?

I really don't know this code at all well, but it doesn't seem to me like any WebKit changes should be necessary.

[Naoki Takano]

Ilya,

I agree we might not need to change WebKit if if we just move the resource.


But, again, I already asked James here

http://code.google.com/p/chromium/issues/detail?id=50685

>2, does "loadPlatformResource" mean "loadResource"? Or do we have to declare another function different from "loadResouce"? And should we use "loadPlatformResource" for icon loading and change the calling in WebKit source code?


And he replied,

>re: loadPlatformResource. No, it means loadPlatformResource. ack-grep is your friend.

I don't know his intention, but I guess he wanted to separate the function according to the resource location.

Thanks,

(In reply to comment #11)
> To be clearer: Do we really need WebKit changes in order to move the resources in Chromium?  If so, why?
> 
> I really don't know this code at all well, but it doesn't seem to me like any WebKit changes should be necessary.

[Ilya Sherman]

(In reply to comment #12)
> Ilya,
> 
> I agree we might not need to change WebKit if if we just move the resource.
> 
> 
> But, again, I already asked James here
> 
> http://code.google.com/p/chromium/issues/detail?id=50685
> 
> >2, does "loadPlatformResource" mean "loadResource"? Or do we have to declare another function different from "loadResouce"? And should we use "loadPlatformResource" for icon loading and change the calling in WebKit source code?
> 
> 
> And he replied,
> 
> >re: loadPlatformResource. No, it means loadPlatformResource. ack-grep is your friend.
> 
> I don't know his intention, but I guess he wanted to separate the function according to the resource location.

It sounds like he meant Image::loadPlatformResource():

"When I said use loadPlatformResource() in the TODO, I mean Image::loadPlatformResource()."

[Naoki Takano]

Ok, so do you agree to change Image::loadPlatformResource()?

(In reply to comment #13)
> (In reply to comment #12)
> > Ilya,
> > 
> > I agree we might not need to change WebKit if if we just move the resource.
> > 
> > 
> > But, again, I already asked James here
> > 
> > http://code.google.com/p/chromium/issues/detail?id=50685
> > 
> > >2, does "loadPlatformResource" mean "loadResource"? Or do we have to declare another function different from "loadResouce"? And should we use "loadPlatformResource" for icon loading and change the calling in WebKit source code?
> > 
> > 
> > And he replied,
> > 
> > >re: loadPlatformResource. No, it means loadPlatformResource. ack-grep is your friend.
> > 
> > I don't know his intention, but I guess he wanted to separate the function according to the resource location.
> 
> It sounds like he meant Image::loadPlatformResource():
> 
> "When I said use loadPlatformResource() in the TODO, I mean Image::loadPlatformResource()."

[Ilya Sherman]

(In reply to comment #14)
> Ok, so do you agree to change Image::loadPlatformResource()?

Could you please explain what change exactly is needed and why?  That might be easiest to do just by uploading the diff and perhaps pointing out the relevant part of it.

Again, I do not know this corner of the code at all well, but it looks to me like we should be able to just use it as-is and still be able to move the image resources, which is the real goal.

[David Holloway]

It appears that:

Image::loadPlatformResource calls
PlatformBridge::loadPlatformImageResource which calls
WebKitClient::loadResource

So we end up at the same place via a different route.

There's no advantage to adding a new method to the WebKitClient interface.

There is little (if any) advantage to taking the Image::loadPlatformResource route.

So I would lobby that we abandon this patch.  However it would be very nice to rename the resources on the Chrome side to be chrome/common/resources/cc_amex.png, chrome/common/resources/cc_generic.png, etc.  But let's discuss that in http://codereview.chromium.org/6516002/.


(In reply to comment #15)
> (In reply to comment #14)
> > Ok, so do you agree to change Image::loadPlatformResource()?
> 
> Could you please explain what change exactly is needed and why?  That might be easiest to do just by uploading the diff and perhaps pointing out the relevant part of it.
> 
> Again, I do not know this corner of the code at all well, but it looks to me like we should be able to just use it as-is and still be able to move the image resources, which is the real goal.