Hi Stig, Thanks for the update.
> I'll start off with a comment about Gears for Opera on other platforms > than WinMob. I see that you very much try to limit the code we write to > only WINCE. I don't think that is something we want from our side. The > Gears code used for Opera will be the same on all platforms ... I didn't mean to suggest that we should limit the Opera code in general to WinCE only. The reason I suggested guarding the OpGearsAPI interface with OS_WINCE was that I assumed it was specific to Opera Mobile. If the API is common to all versions of Opera, that's great and of course we shouldn't limit it to WinCE. Sorry for the confusion. > It would benefit us greatly if we didn't have to build our own versions of > the Gears plugin for our desktop releases but instead use your newest and > finest code from the repository. If we keep the Opera desktop build code > in the official branch we can do that, even if you decide not to put them > on your page as an official release yet. Of course, that sounds reasonable. > We will, of course, do all we can > not to cripple the Chrome builds for desktop when making it compile for > us, and hope you can do the same the other way around. I'm sure we can make that work. > And now for the changes I have made according to your feedback. The patches > attached overrides the ones I sent for the same files last time. Thanks for this. The patches looks great. >> File >> //depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_utils.cc >> (snapshot 3) >> ------------------------------------ >> Line 134: SettingsDialog::Run(NULL); >> Is there any need for this method to be part of OperaUtils? > > Not really, but it is for convenience. A function that calls Run(NULL) > must exist somewhere because we don't want to export the notion of > platform_data across the Opera/Gears interface, and why not in Opera utils. OK, sounds good. >> File >> //depot/googleclient/gears/opensource/gears/base/opera_mobile/opera_utils.h >> (snapshot 1) >> ------------------------------------ >> Line 39: static OpGearsAPI* GetGearsApi(); >> From the perspective of Gears, 'GetBrowserApiForGears' or similar makes >> more sense. > Well, you suggested GetGearsApi(), but if you think > GetBrowserApiForGears() is better we'll go with that. Sorry about that, at the time I didn't appreciate exactly how the API pointer is used. >> ------------------------------------ >> Line 43: }; >> All classes should use DISALLOW_EVIL_CONSTRUCTORS; > > All classes? I found several of your existing classes not doing that, for > instance SyncFunctor and BrowsingContext. I couldn't find one in the style > guide but is there a rule about when to and when not to use it? DISALLOW_EVIL_CONSTRUCTORS is specific to Gears. The equivalent in the style guide is DISALLOW_COPY_AND_ASSIGN. It should be used for all classes where we don't need the default copy constructor and assignment operator. If it's missing from any Gears classes, it's likely to be a Gears error. >> File >> //depot/googleclient/gears/opensource/third_party/opera_mobile/opera_callback_api.h >> (snapshot 5) >> ------------------------------------ >> Line 12: typedef void (*OP_Initialize_func)(OpGearsAPI *, OperaCallbacks >> *); >> This type isn't used. > > It is used in Opera, and since the files are used in both sources I think > I'll keep it. I see, I didn't realise that Opera shares this header with Gears. The only comments I have about the latest version of the patches are concerned with style. I think I've mentioned them all already, but to recap, the main ones are ... - No tabs in code. - Interfaces are named XXXInterface - Class names use camel caps, even for acronyms, eg OpHttpListener - For the Opera API classes, let's use 'Opera' rather than 'Op' (Though cross-browser classes use a prefix of 'OP' - OPHttpRequest, IEHttpRequest, FFHttpRequest etc) I'm happy to make these style changes to the current changelist myself, to save another round of patching, but it would be good to note these things for future patches. Let me know if you'd like me to do this. I'll mail out the latest version of this changelist, as well as a new changelist to add OPBrowsingContext. Thanks, Steve -- Google UK Limited Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ Registered in England Number: 3977902
