Since I am away, Steve and me chatted about this CL. It is ok, the only major item is putting the VersionFecthTask class back into the cpp.
There were some other things discussed: see below if you are interested. -------------------------------------------------------------------------- me: steve, looking at 8762257 why this change at all? Steve: because VersionChecker was using a feature of AsyncTask - the listener window - which is specific to the IE implementation. See periodic_checker.cc line 371 me: NotifyListener? how is that specific to IE? me: so to me, this change only complicates things I'm not sure I see why we need it Steve: The 'NotifyListener' call itself is common to all implementations, but the method of specifying the listener is not. See SetListenerWindow in async_task_ie.h and SetListener in async_task_np.h. Steve: So the Version checker would have to use different code to set the listener and to receive callbacks for the two browsers (IE and NPAPI) me: right. so actually SetListenerWindows isn't IE specific at all it is win32 specific except that async_task_np doesn't use it so you reimplemented the win32 notification mechanism in the class that derives from AsyncTask but since this is for the setup.dll only I wonder if we can't define BROWSER_IE for it rather than re-implementing the same thing? ah, it's not setup.dll. it's the installer component Steve: Yes, when I said SetListenerWindow is IE-specific, I meant that it's in the IE-specific implementation of AsyncTask me: yes, but that would work on Opera just fine, right? Steve: yes, it would so you mean modify async_task_np to provide SetListenerWindow when built on Win32? me: technically, the VersionFetchtask should use async_task_ie Steve: why? me: because that's the technically correct solution on win32 Steve: or do you mean that async_task_ie should really be called async_task_win32? me: yes Steve: i agree, you're right. but i don't want to start messing with async task me: sure but you see what situation this leads to? Steve: and i think my solution is just as 'correct', if a little less obvious me: we have the functionality we want in AsyncTask (ie version) but now we have to re-implement it in the child class! Steve: plus it means that the PeriodicChecker is also platform-indepenedent, as well as browser-independent me: no it's not Steve: i don't reimplement the window messaging in the child class. the child class uses a callback. the owning class does the window message stuff. sorry, PeriodicChecker -> VersionFetchTask VersionFetchTask is platform independent me: no it's not u'd have to factor out ExtractVersionAndDownloadUrl Steve: ah, ok, i'd forgotten that the xml parsing is win32-specific me: that can be factored out easily into a static function inside a platform-specific cpp same with periodic checker if you really want to make it platform independent Steve: yup me: maybe put the VersionFetchTask into a separate file but I'm not sure we should aim to be platform independent Steve: no, i wasn't aiming to be platform independent. me: ok Steve: but i needed it to work on NPAPI, didn't want to mess with async task, and thought i should head in the direction of platform-independence where possible the real problem is that async task (and http request) are odd mixtures of being platform- and browser-specific but i don't think we want to try to fix that now me: yes...agreed me: but I just don't like the fact that although the base class' declared goal is to "provide a means to send notification messages to a listener", we end up ignoring this functionality, then using the NP version of the base class and then copy&paste the notification mechanism from the IE version me: but I guess you're right in saying that fixing it right would affect so much of core gears functionality that it's probably better to leave it Steve: i can't even include async_task_ie.h directly (rather than through async_task.h) and build both async_task_ie.cc and async_task_np.cc because they both have the class name AsyncTask i guess i'll just add a big comment? me: yeah, ok me: also, maybe you could avoid making the VersionFetchTask part of the Checker interface? thus: fwd-declare some VersionFetchTaskListener class have the PeriodicChecker own a pte to this listener class in the CPP, declare the class and make it derive from the versionFetchTaskListener::ListenerInterface would that work? Steve: yeah makes sense me: long live composition Steve: the impl of the listener would probably have to be a friend of the checker too me: sure, but that would work with the fwd-declaration Steve: yup -- To respond, reply to this email or visit http://mondrian.corp.google.com/8762257
