Adding a privileged callback used in IE CF to check whether to show...

Adding a privileged callback used in IE CF to check whether to show
the version mismatch warning dialog. Used this in ceee/ to only show
it once per tab.

Changed the logic in Firefox to show the warning dialog even when
in privileged mode. This will mean it gets shown once per Firefox
window.

Wrote a unit test for the additional logic in ChromeFrameActivex.

To write the unit test, used com_mock.py to generate a mock of
the IChromeFramePrivileged interface. This can be extended to
generate mocks of the other CF interfaces.

Discovered duplication of np_browser_functions.h and .cc, resolved
this to a single copy (the one under chrome_frame).

Changed things around so chrome_tab.idl is built only once; this
also lets me more easily depend on it in the com_mock rule.

BUG=none
TEST=chrome_frame_unittests.exe



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

[robertshield, 2010-11-05 13:45:29]

Nice stuff, some comments below.

http://codereview.chromium.org/4563001/diff/1/11
File ceee/testing/utils/mock_com.h (right):

http://codereview.chromium.org/4563001/diff/1/11#newcode29
ceee/testing/utils/mock_com.h:29: #include
"ceee/testing/utils/mock_ioleclientsite.gen"
This is cool, but since the .gen file isn't being udpated automatically, could
we just paste the generated mock method definitions here and avoid the inline
#include?

http://codereview.chromium.org/4563001/diff/1/13
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/4563001/diff/1/13#newcode97
chrome_frame/chrome_frame.gyp:97:
'<(SHARED_INTERMEDIATE_DIR)/mock_ichromeframeprivileged.gen',
why is this both a source and an output? won't that cause this action to run on
every build whether it needs to or not?

http://codereview.chromium.org/4563001/diff/1/13#newcode105
chrome_frame/chrome_frame.gyp:105: '../ceee/testing/utils/com_mock.py',
Along the lines of the previous comment, scripts that are run as actions are
supposed to, once they've computed their output, check it against any output
file already present in the build directory. 

If the output file is already binary-identical to what would be written then no
writes should take place to avoid triggering a rebuild of any other targets that
depend on the output. This would require changes to com_mock.py but is
definitely worth doing.

http://codereview.chromium.org/4563001/diff/1/19
File chrome_frame/test/chrome_frame_activex_unittest.cc (right):

http://codereview.chromium.org/4563001/diff/1/19#newcode56
chrome_frame/test/chrome_frame_activex_unittest.cc:56: // If not privileged, we
always show the dialog.
your tests rock. that is all.

http://codereview.chromium.org/4563001/diff/1/20
File chrome_frame/test/chrome_tab_mocks.h (right):

http://codereview.chromium.org/4563001/diff/1/20#newcode20
chrome_frame/test/chrome_tab_mocks.h:20: #include
"mock_ichromeframeprivileged.gen"  // NOLINT
I'm more partial to generating .h files that define macros than inline
#includes. The macros mess with debuggers/IDEs a bit less when stepping through
code, but this is ok if you strongly prefer it.

[Jói, 2010-11-05 17:02:01]

Thanks Robert. I'll upload a new revision later today but wanted to respond to
comments now.

http://codereview.chromium.org/4563001/diff/1/11
File ceee/testing/utils/mock_com.h (right):

http://codereview.chromium.org/4563001/diff/1/11#newcode29
ceee/testing/utils/mock_com.h:29: #include
"ceee/testing/utils/mock_ioleclientsite.gen"
On 2010/11/05 13:45:29, robertshield wrote:
> This is cool, but since the .gen file isn't being udpated automatically, could
> we just paste the generated mock method definitions here and avoid the inline
> #include?

We could, I'm keeping consistent with this file.

I'd like to check in as-is if it's OK. I was thinking of revamping this whole
mock generation business, now that there is an established pattern of using it,
so that the tool outputs full header files with an IFooMockImpl and a MockIFoo
object as per the pattern you see in this file.

This file could then limit its definitions to objects that use more than one
MockImpl, such as MockIWebBrowser2.

http://codereview.chromium.org/4563001/diff/1/13
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/4563001/diff/1/13#newcode97
chrome_frame/chrome_frame.gyp:97:
'<(SHARED_INTERMEDIATE_DIR)/mock_ichromeframeprivileged.gen',
On 2010/11/05 13:45:29, robertshield wrote:
> why is this both a source and an output? won't that cause this action to run
on
> every build whether it needs to or not?

I think this may be wrong, I copied the idea from generation steps in
ceee/testing/utils. My sleep-deprived brain at the time told me this might be
weird, but I justified it in terms of "well, if anybody modifies the output
file, you'd want to regenerate it, right?" :-)

The try bot also complained on something related to this rule so I'll take a
better look.

http://codereview.chromium.org/4563001/diff/1/13#newcode105
chrome_frame/chrome_frame.gyp:105: '../ceee/testing/utils/com_mock.py',
On 2010/11/05 13:45:29, robertshield wrote:
> Along the lines of the previous comment, scripts that are run as actions are
> supposed to, once they've computed their output, check it against any output
> file already present in the build directory. 
> 
> If the output file is already binary-identical to what would be written then
no
> writes should take place to avoid triggering a rebuild of any other targets
that
> depend on the output. This would require changes to com_mock.py but is
> definitely worth doing.

I will submit that this should be a GYP feature, it would be easy to generalize
for any code generation steps.

That being said, I'll put a TODO in com_mock.py to fix this at the same time I
revamp the generation of code as mentioned in my other comment response.

http://codereview.chromium.org/4563001/diff/1/19
File chrome_frame/test/chrome_frame_activex_unittest.cc (right):

http://codereview.chromium.org/4563001/diff/1/19#newcode56
chrome_frame/test/chrome_frame_activex_unittest.cc:56: // If not privileged, we
always show the dialog.
On 2010/11/05 13:45:29, robertshield wrote:
> your tests rock. that is all.

Done.

http://codereview.chromium.org/4563001/diff/1/20
File chrome_frame/test/chrome_tab_mocks.h (right):

http://codereview.chromium.org/4563001/diff/1/20#newcode20
chrome_frame/test/chrome_tab_mocks.h:20: #include
"mock_ichromeframeprivileged.gen"  // NOLINT
On 2010/11/05 13:45:29, robertshield wrote:
> I'm more partial to generating .h files that define macros than inline
> #includes. The macros mess with debuggers/IDEs a bit less when stepping
through
> code, but this is ok if you strongly prefer it.

I agree in most cases, will include this in my TODO around com_mock revamp.

In this case the reason was that it wasn't generating full header files at all -
but that will change with the revamp.

[robertshield, 2010-11-05 17:22:25]

OK, LGTM with the addition of a TODO() to rejigger the mock generation to not
require inline includes and check for unchanged output files as well as figuring
out what's up with the target with the same source and output file.

http://codereview.chromium.org/4563001/diff/1/11
File ceee/testing/utils/mock_com.h (right):

http://codereview.chromium.org/4563001/diff/1/11#newcode29
ceee/testing/utils/mock_com.h:29: #include
"ceee/testing/utils/mock_ioleclientsite.gen"
On 2010/11/05 17:02:01, Jói wrote:
> On 2010/11/05 13:45:29, robertshield wrote:
> > This is cool, but since the .gen file isn't being udpated automatically,
could
> > we just paste the generated mock method definitions here and avoid the
inline
> > #include?
> 
> We could, I'm keeping consistent with this file.
> 
> I'd like to check in as-is if it's OK. I was thinking of revamping this whole
> mock generation business, now that there is an established pattern of using
it,
> so that the tool outputs full header files with an IFooMockImpl and a MockIFoo
> object as per the pattern you see in this file.
> 
> This file could then limit its definitions to objects that use more than one
> MockImpl, such as MockIWebBrowser2.

Ok, the described revamp sounds like a Good Thing to me. I'm fine with this as
is, I guess I was mainly musing :-)

http://codereview.chromium.org/4563001/diff/1/13
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/4563001/diff/1/13#newcode97
chrome_frame/chrome_frame.gyp:97:
'<(SHARED_INTERMEDIATE_DIR)/mock_ichromeframeprivileged.gen',
On 2010/11/05 17:02:01, Jói wrote:
> On 2010/11/05 13:45:29, robertshield wrote:
> > why is this both a source and an output? won't that cause this action to run
> on
> > every build whether it needs to or not?
> 
> I think this may be wrong, I copied the idea from generation steps in
> ceee/testing/utils. My sleep-deprived brain at the time told me this might be
> weird, but I justified it in terms of "well, if anybody modifies the output
> file, you'd want to regenerate it, right?" :-)
> 
> The try bot also complained on something related to this rule so I'll take a
> better look.

Ok, let me know what you find out, I'd be surprised if a target with the same
source and output file was the right thing to do :-)

http://codereview.chromium.org/4563001/diff/1/13#newcode105
chrome_frame/chrome_frame.gyp:105: '../ceee/testing/utils/com_mock.py',
On 2010/11/05 17:02:01, Jói wrote:
> On 2010/11/05 13:45:29, robertshield wrote:
> > Along the lines of the previous comment, scripts that are run as actions are
> > supposed to, once they've computed their output, check it against any output
> > file already present in the build directory. 
> > 
> > If the output file is already binary-identical to what would be written then
> no
> > writes should take place to avoid triggering a rebuild of any other targets
> that
> > depend on the output. This would require changes to com_mock.py but is
> > definitely worth doing.
> 
> I will submit that this should be a GYP feature, it would be easy to
generalize
> for any code generation steps.
> 

This is true. 

> That being said, I'll put a TODO in com_mock.py to fix this at the same time I
> revamp the generation of code as mentioned in my other comment response.

sgtm.

[amit, 2010-11-05 17:23:04]

drive by...

http://codereview.chromium.org/4563001/diff/1/14
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/4563001/diff/1/14#newcode276
chrome_frame/chrome_frame_activex.cc:276: THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
nit: we should record UMA version mismatch irrespective of
ShouldShowVersionMismatchDialog

http://codereview.chromium.org/4563001/diff/1/17
File chrome_frame/chrome_tab.idl (right):

http://codereview.chromium.org/4563001/diff/1/17#newcode119
chrome_frame/chrome_tab.idl:119: HRESULT ShouldShowVersionMismatchDialog();
Would it be better to give this control to the privileged client? So instead of
ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event (through
DIChromeFrameEvents or adding OnVersionMismatch here if that's too much
trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the dialog only if
the event is not handled successfully.

[Jói, 2010-11-05 17:29:51]

Thanks Amit, see responses below.

http://codereview.chromium.org/4563001/diff/1/14
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/4563001/diff/1/14#newcode276
chrome_frame/chrome_frame_activex.cc:276: THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
On 2010/11/05 17:23:04, amit wrote:
> nit: we should record UMA version mismatch irrespective of
> ShouldShowVersionMismatchDialog 

The count name is "VersionMismatchDisplayed" (note the last bit) which is why I
didn't increment it (that and the previous code didn't either). Let me know
which is the right thing to do.

http://codereview.chromium.org/4563001/diff/1/17
File chrome_frame/chrome_tab.idl (right):

http://codereview.chromium.org/4563001/diff/1/17#newcode119
chrome_frame/chrome_tab.idl:119: HRESULT ShouldShowVersionMismatchDialog();
On 2010/11/05 17:23:04, amit wrote:
> Would it be better to give this control to the privileged client? So instead
of
> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event (through
> DIChromeFrameEvents or adding OnVersionMismatch here if that's too much
> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the dialog only
if
> the event is not handled successfully.

I think the current approach gives maximum flexibility as well as reuse of the
existing dialog code.

If you always want to take over showing a dialog or other UI, you always respond
S_FALSE and do your own thing. If you want to reuse the current dialog, you
respond S_OK in the cases where you want it.

[amit, 2010-11-05 17:47:14]

On Fri, Nov 5, 2010 at 10:29 AM, <joi@chromium.org> wrote:

> Thanks Amit, see responses below.
>
>
>
> http://codereview.chromium.org/4563001/diff/1/14
> File chrome_frame/chrome_frame_activex.cc (right):
>
> http://codereview.chromium.org/4563001/diff/1/14#newcode276
> chrome_frame/chrome_frame_activex.cc:276:
> THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
> On 2010/11/05 17:23:04, amit wrote:
>
>> nit: we should record UMA version mismatch irrespective of
>> ShouldShowVersionMismatchDialog
>>
>
> The count name is "VersionMismatchDisplayed" (note the last bit) which
> is why I didn't increment it (that and the previous code didn't either).
> Let me know which is the right thing to do.

Robert can have a final say here, but I think the intent here is to measure
how many times the version mismatch is happening so that we can see
effectiveness of newer versioning scheme over the old one.  Maybe the right
thing to do is change 'VersionMismatchDisplayed' to just VersionMismatch.
But, I guess that's beyond the scope of this CL.


>
>
> http://codereview.chromium.org/4563001/diff/1/17
> File chrome_frame/chrome_tab.idl (right):
>
> http://codereview.chromium.org/4563001/diff/1/17#newcode119
> chrome_frame/chrome_tab.idl:119: HRESULT
> ShouldShowVersionMismatchDialog();
> On 2010/11/05 17:23:04, amit wrote:
>
>> Would it be better to give this control to the privileged client? So
>>
> instead of
>
>> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event
>>
> (through
>
>> DIChromeFrameEvents or adding OnVersionMismatch here if that's too
>>
> much
>
>> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the
>>
> dialog only if
>
>> the event is not handled successfully.
>>
>
> I think the current approach gives maximum flexibility as well as reuse
> of the existing dialog code.
>
> If you always want to take over showing a dialog or other UI, you always
> respond S_FALSE and do your own thing. If you want to reuse the current
> dialog, you respond S_OK in the cases where you want it.


I think that makes sense and essentially ShouldShowVersionMismatchDialog is
very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
this notification allows other customers including Javascript and tests
to handle this uniformly.


>
> http://codereview.chromium.org/4563001/show
>

[robertshield, 2010-11-05 18:02:20]

On Fri, Nov 5, 2010 at 1:47 PM, Amit Joshi <amit@chromium.org> wrote:

>
>
> On Fri, Nov 5, 2010 at 10:29 AM, <joi@chromium.org> wrote:
>
>> Thanks Amit, see responses below.
>>
>>
>>
>> http://codereview.chromium.org/4563001/diff/1/14
>> File chrome_frame/chrome_frame_activex.cc (right):
>>
>> http://codereview.chromium.org/4563001/diff/1/14#newcode276
>> chrome_frame/chrome_frame_activex.cc:276:
>> THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
>> On 2010/11/05 17:23:04, amit wrote:
>>
>>> nit: we should record UMA version mismatch irrespective of
>>> ShouldShowVersionMismatchDialog
>>>
>>
>> The count name is "VersionMismatchDisplayed" (note the last bit) which
>> is why I didn't increment it (that and the previous code didn't either).
>> Let me know which is the right thing to do.
>
> Robert can have a final say here, but I think the intent here is to measure
> how many times the version mismatch is happening so that we can see
> effectiveness of newer versioning scheme over the old one.  Maybe the right
> thing to do is change 'VersionMismatchDisplayed' to just VersionMismatch.
> But, I guess that's beyond the scope of this CL.
>

The intent *today* was to measure how often the dialog displayed to see how
badly we are bugging users. Measuring how often we hit a mismatch in total
is also a good metric to have, but it's not there yet. I should add it. I
would say that this CL is fine as is and that I should go and update the
metrics properly in another CL.

I in fact need to go and fix this up anyway, because I just had a big
face-palm moment: it looks like the active doc isn't showing the dialog,
only the activex and the npapi plugin. This needs fixing and better
testing...


>
>
>>
>>
>> http://codereview.chromium.org/4563001/diff/1/17
>> File chrome_frame/chrome_tab.idl (right):
>>
>> http://codereview.chromium.org/4563001/diff/1/17#newcode119
>> chrome_frame/chrome_tab.idl:119: HRESULT
>> ShouldShowVersionMismatchDialog();
>> On 2010/11/05 17:23:04, amit wrote:
>>
>>> Would it be better to give this control to the privileged client? So
>>>
>> instead of
>>
>>> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event
>>>
>> (through
>>
>>> DIChromeFrameEvents or adding OnVersionMismatch here if that's too
>>>
>> much
>>
>>> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the
>>>
>> dialog only if
>>
>>> the event is not handled successfully.
>>>
>>
>> I think the current approach gives maximum flexibility as well as reuse
>> of the existing dialog code.
>>
>> If you always want to take over showing a dialog or other UI, you always
>> respond S_FALSE and do your own thing. If you want to reuse the current
>> dialog, you respond S_OK in the cases where you want it.
>
>
> I think that makes sense and essentially ShouldShowVersionMismatchDialog is
> very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
> this notification allows other customers including Javascript and tests
> to handle this uniformly.
>
>
... and speaking of testing, the approach in this CL looks simpler IMO for
the kind of tests I would want to write, but I'll let you two more
testing-knowledgeable folk debate it out :-)


>
>>
>> http://codereview.chromium.org/4563001/show
>>
>
>

[Jói, 2010-11-05 19:53:16]

> I think that makes sense and essentially ShouldShowVersionMismatchDialog is
> very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
> this notification allows other customers including Javascript and tests
> to handle this uniformly.

I'm not sure whether you're asking me to change it before checking in
or not, please let me know. I am willing change it if you think you
want this in the public interface, it's a fair amount of extra work
but the if it's the right thing, then it's the right thing.

Cheers,
Jói



On Fri, Nov 5, 2010 at 1:47 PM, Amit Joshi <amit@chromium.org> wrote:
>
>
> On Fri, Nov 5, 2010 at 10:29 AM, <joi@chromium.org> wrote:
>>
>> Thanks Amit, see responses below.
>>
>>
>> http://codereview.chromium.org/4563001/diff/1/14
>> File chrome_frame/chrome_frame_activex.cc (right):
>>
>> http://codereview.chromium.org/4563001/diff/1/14#newcode276
>> chrome_frame/chrome_frame_activex.cc:276:
>> THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
>> On 2010/11/05 17:23:04, amit wrote:
>>>
>>> nit: we should record UMA version mismatch irrespective of
>>> ShouldShowVersionMismatchDialog
>>
>> The count name is "VersionMismatchDisplayed" (note the last bit) which
>> is why I didn't increment it (that and the previous code didn't either).
>> Let me know which is the right thing to do.
>
> Robert can have a final say here, but I think the intent here is to measure
> how many times the version mismatch is happening so that we can see
> effectiveness of newer versioning scheme over the old one.  Maybe the right
> thing to do is change 'VersionMismatchDisplayed' to just VersionMismatch.
> But, I guess that's beyond the scope of this CL.
>
>>
>> http://codereview.chromium.org/4563001/diff/1/17
>> File chrome_frame/chrome_tab.idl (right):
>>
>> http://codereview.chromium.org/4563001/diff/1/17#newcode119
>> chrome_frame/chrome_tab.idl:119: HRESULT
>> ShouldShowVersionMismatchDialog();
>> On 2010/11/05 17:23:04, amit wrote:
>>>
>>> Would it be better to give this control to the privileged client? So
>>
>> instead of
>>>
>>> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event
>>
>> (through
>>>
>>> DIChromeFrameEvents or adding OnVersionMismatch here if that's too
>>
>> much
>>>
>>> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the
>>
>> dialog only if
>>>
>>> the event is not handled successfully.
>>
>> I think the current approach gives maximum flexibility as well as reuse
>> of the existing dialog code.
>>
>> If you always want to take over showing a dialog or other UI, you always
>> respond S_FALSE and do your own thing. If you want to reuse the current
>> dialog, you respond S_OK in the cases where you want it.
>
> I think that makes sense and essentially ShouldShowVersionMismatchDialog is
> very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
> this notification allows other customers including Javascript and tests
> to handle this uniformly.
>>
>>
>> http://codereview.chromium.org/4563001/show
>
>

[amit, 2010-11-05 21:46:46]

On Fri, Nov 5, 2010 at 12:52 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> > I think that makes sense and essentially ShouldShowVersionMismatchDialog
> is
> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
> > this notification allows other customers including Javascript and tests
> > to handle this uniformly.
>
> I'm not sure whether you're asking me to change it before checking in
> or not, please let me know. I am willing change it if you think you
> want this in the public interface, it's a fair amount of extra work
> but the if it's the right thing, then it's the right thing.
>

IMO, that's the right thing since it's of a generic  value. Hopefully it
shouldn't be a lot of extra work (follow how "CloseWindow" event is
implemented).


> Cheers,
> Jói
>
>
>
> On Fri, Nov 5, 2010 at 1:47 PM, Amit Joshi <amit@chromium.org> wrote:
> >
> >
> > On Fri, Nov 5, 2010 at 10:29 AM, <joi@chromium.org> wrote:
> >>
> >> Thanks Amit, see responses below.
> >>
> >>
> >> http://codereview.chromium.org/4563001/diff/1/14
> >> File chrome_frame/chrome_frame_activex.cc (right):
> >>
> >> http://codereview.chromium.org/4563001/diff/1/14#newcode276
> >> chrome_frame/chrome_frame_activex.cc:276:
> >> THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
> >> On 2010/11/05 17:23:04, amit wrote:
> >>>
> >>> nit: we should record UMA version mismatch irrespective of
> >>> ShouldShowVersionMismatchDialog
> >>
> >> The count name is "VersionMismatchDisplayed" (note the last bit) which
> >> is why I didn't increment it (that and the previous code didn't either).
> >> Let me know which is the right thing to do.
> >
> > Robert can have a final say here, but I think the intent here is to
> measure
> > how many times the version mismatch is happening so that we can see
> > effectiveness of newer versioning scheme over the old one.  Maybe the
> right
> > thing to do is change 'VersionMismatchDisplayed' to just VersionMismatch.
> > But, I guess that's beyond the scope of this CL.
> >
> >>
> >> http://codereview.chromium.org/4563001/diff/1/17
> >> File chrome_frame/chrome_tab.idl (right):
> >>
> >> http://codereview.chromium.org/4563001/diff/1/17#newcode119
> >> chrome_frame/chrome_tab.idl:119: HRESULT
> >> ShouldShowVersionMismatchDialog();
> >> On 2010/11/05 17:23:04, amit wrote:
> >>>
> >>> Would it be better to give this control to the privileged client? So
> >>
> >> instead of
> >>>
> >>> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event
> >>
> >> (through
> >>>
> >>> DIChromeFrameEvents or adding OnVersionMismatch here if that's too
> >>
> >> much
> >>>
> >>> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the
> >>
> >> dialog only if
> >>>
> >>> the event is not handled successfully.
> >>
> >> I think the current approach gives maximum flexibility as well as reuse
> >> of the existing dialog code.
> >>
> >> If you always want to take over showing a dialog or other UI, you always
> >> respond S_FALSE and do your own thing. If you want to reuse the current
> >> dialog, you respond S_OK in the cases where you want it.
> >
> > I think that makes sense and essentially ShouldShowVersionMismatchDialog
> is
> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
> > this notification allows other customers including Javascript and tests
> > to handle this uniformly.
> >>
> >>
> >> http://codereview.chromium.org/4563001/show
> >
> >
>

[Jói, 2010-11-05 23:41:23]

OK. Do you mind if I leave the existing mechanism for determining
whether to show the dialog, and just add the DIChromeFrameEvents
notification? The current mechanism seems the simplest way for our
internal implementation to have the functionality we want, and the
DIChromeFrameEvents is more useful for public interface users; e.g. I
don't think we should let users of the public interface dictate
whether the dialog is shown. Happy to discuss further by phone.

Cheers,
Jói


2010/11/5 Amit Joshi <amit@chromium.org>:
>
> On Fri, Nov 5, 2010 at 12:52 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> > I think that makes sense and essentially ShouldShowVersionMismatchDialog
>> > is
>> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
>> > this notification allows other customers including Javascript and tests
>> > to handle this uniformly.
>>
>> I'm not sure whether you're asking me to change it before checking in
>> or not, please let me know. I am willing change it if you think you
>> want this in the public interface, it's a fair amount of extra work
>> but the if it's the right thing, then it's the right thing.
>
> IMO, that's the right thing since it's of a generic  value. Hopefully it
> shouldn't be a lot of extra work (follow how "CloseWindow" event is
> implemented).
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Fri, Nov 5, 2010 at 1:47 PM, Amit Joshi <amit@chromium.org> wrote:
>> >
>> >
>> > On Fri, Nov 5, 2010 at 10:29 AM, <joi@chromium.org> wrote:
>> >>
>> >> Thanks Amit, see responses below.
>> >>
>> >>
>> >> http://codereview.chromium.org/4563001/diff/1/14
>> >> File chrome_frame/chrome_frame_activex.cc (right):
>> >>
>> >> http://codereview.chromium.org/4563001/diff/1/14#newcode276
>> >> chrome_frame/chrome_frame_activex.cc:276:
>> >> THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
>> >> On 2010/11/05 17:23:04, amit wrote:
>> >>>
>> >>> nit: we should record UMA version mismatch irrespective of
>> >>> ShouldShowVersionMismatchDialog
>> >>
>> >> The count name is "VersionMismatchDisplayed" (note the last bit) which
>> >> is why I didn't increment it (that and the previous code didn't
>> >> either).
>> >> Let me know which is the right thing to do.
>> >
>> > Robert can have a final say here, but I think the intent here is to
>> > measure
>> > how many times the version mismatch is happening so that we can see
>> > effectiveness of newer versioning scheme over the old one.  Maybe the
>> > right
>> > thing to do is change 'VersionMismatchDisplayed' to just
>> > VersionMismatch.
>> > But, I guess that's beyond the scope of this CL.
>> >
>> >>
>> >> http://codereview.chromium.org/4563001/diff/1/17
>> >> File chrome_frame/chrome_tab.idl (right):
>> >>
>> >> http://codereview.chromium.org/4563001/diff/1/17#newcode119
>> >> chrome_frame/chrome_tab.idl:119: HRESULT
>> >> ShouldShowVersionMismatchDialog();
>> >> On 2010/11/05 17:23:04, amit wrote:
>> >>>
>> >>> Would it be better to give this control to the privileged client? So
>> >>
>> >> instead of
>> >>>
>> >>> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event
>> >>
>> >> (through
>> >>>
>> >>> DIChromeFrameEvents or adding OnVersionMismatch here if that's too
>> >>
>> >> much
>> >>>
>> >>> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the
>> >>
>> >> dialog only if
>> >>>
>> >>> the event is not handled successfully.
>> >>
>> >> I think the current approach gives maximum flexibility as well as reuse
>> >> of the existing dialog code.
>> >>
>> >> If you always want to take over showing a dialog or other UI, you
>> >> always
>> >> respond S_FALSE and do your own thing. If you want to reuse the current
>> >> dialog, you respond S_OK in the cases where you want it.
>> >
>> > I think that makes sense and essentially ShouldShowVersionMismatchDialog
>> > is
>> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents for
>> > this notification allows other customers including Javascript and tests
>> > to handle this uniformly.
>> >>
>> >>
>> >> http://codereview.chromium.org/4563001/show
>> >
>> >
>
>

[amit, 2010-11-05 23:50:48]

On Fri, Nov 5, 2010 at 4:40 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> OK. Do you mind if I leave the existing mechanism for determining
> whether to show the dialog, and just add the DIChromeFrameEvents
> notification? The current mechanism seems the simplest way for our
> internal implementation to have the functionality we want, and the
> DIChromeFrameEvents is more useful for public interface users; e.g. I
> don't think we should let users of the public interface dictate
> whether the dialog is shown. Happy to discuss further by phone.
>

So the ActiveDoc and Privileged interface users are binary embedders and
it's fine to have them say over what needs to happen. For Activex/NPAPI
within a page, it's better if the developer knows that GCF is not going to
function due to version mismatch. In that case she might want to display a
better message on the page instead of the message box?

In any case, if others are blocked on this, feel free to land it and address
it in the subsequent CLs.


> Cheers,
> Jói
>
>
> 2010/11/5 Amit Joshi <amit@chromium.org>:
> >
> > On Fri, Nov 5, 2010 at 12:52 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>
> >> > I think that makes sense and
> essentially ShouldShowVersionMismatchDialog
> >> > is
> >> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents
> for
> >> > this notification allows other customers including Javascript and
> tests
> >> > to handle this uniformly.
> >>
> >> I'm not sure whether you're asking me to change it before checking in
> >> or not, please let me know. I am willing change it if you think you
> >> want this in the public interface, it's a fair amount of extra work
> >> but the if it's the right thing, then it's the right thing.
> >
> > IMO, that's the right thing since it's of a generic  value. Hopefully it
> > shouldn't be a lot of extra work (follow how "CloseWindow" event is
> > implemented).
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >>
> >> On Fri, Nov 5, 2010 at 1:47 PM, Amit Joshi <amit@chromium.org> wrote:
> >> >
> >> >
> >> > On Fri, Nov 5, 2010 at 10:29 AM, <joi@chromium.org> wrote:
> >> >>
> >> >> Thanks Amit, see responses below.
> >> >>
> >> >>
> >> >> http://codereview.chromium.org/4563001/diff/1/14
> >> >> File chrome_frame/chrome_frame_activex.cc (right):
> >> >>
> >> >> http://codereview.chromium.org/4563001/diff/1/14#newcode276
> >> >> chrome_frame/chrome_frame_activex.cc:276:
> >> >> THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
> >> >> On 2010/11/05 17:23:04, amit wrote:
> >> >>>
> >> >>> nit: we should record UMA version mismatch irrespective of
> >> >>> ShouldShowVersionMismatchDialog
> >> >>
> >> >> The count name is "VersionMismatchDisplayed" (note the last bit)
> which
> >> >> is why I didn't increment it (that and the previous code didn't
> >> >> either).
> >> >> Let me know which is the right thing to do.
> >> >
> >> > Robert can have a final say here, but I think the intent here is to
> >> > measure
> >> > how many times the version mismatch is happening so that we can see
> >> > effectiveness of newer versioning scheme over the old one.  Maybe the
> >> > right
> >> > thing to do is change 'VersionMismatchDisplayed' to just
> >> > VersionMismatch.
> >> > But, I guess that's beyond the scope of this CL.
> >> >
> >> >>
> >> >> http://codereview.chromium.org/4563001/diff/1/17
> >> >> File chrome_frame/chrome_tab.idl (right):
> >> >>
> >> >> http://codereview.chromium.org/4563001/diff/1/17#newcode119
> >> >> chrome_frame/chrome_tab.idl:119: HRESULT
> >> >> ShouldShowVersionMismatchDialog();
> >> >> On 2010/11/05 17:23:04, amit wrote:
> >> >>>
> >> >>> Would it be better to give this control to the privileged client? So
> >> >>
> >> >> instead of
> >> >>>
> >> >>> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event
> >> >>
> >> >> (through
> >> >>>
> >> >>> DIChromeFrameEvents or adding OnVersionMismatch here if that's too
> >> >>
> >> >> much
> >> >>>
> >> >>> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the
> >> >>
> >> >> dialog only if
> >> >>>
> >> >>> the event is not handled successfully.
> >> >>
> >> >> I think the current approach gives maximum flexibility as well as
> reuse
> >> >> of the existing dialog code.
> >> >>
> >> >> If you always want to take over showing a dialog or other UI, you
> >> >> always
> >> >> respond S_FALSE and do your own thing. If you want to reuse the
> current
> >> >> dialog, you respond S_OK in the cases where you want it.
> >> >
> >> > I think that makes sense and
> essentially ShouldShowVersionMismatchDialog
> >> > is
> >> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents
> for
> >> > this notification allows other customers including Javascript and
> tests
> >> > to handle this uniformly.
> >> >>
> >> >>
> >> >> http://codereview.chromium.org/4563001/show
> >> >
> >> >
> >
> >
>

[amit, 2010-11-05 23:53:25]

On Fri, Nov 5, 2010 at 4:50 PM, Amit Joshi <amit@chromium.org> wrote:

>
> On Fri, Nov 5, 2010 at 4:40 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> OK. Do you mind if I leave the existing mechanism for determining
>> whether to show the dialog, and just add the DIChromeFrameEvents
>> notification? The current mechanism seems the simplest way for our
>> internal implementation to have the functionality we want,
>
> I thought CEEE subscribed to DIChromeFrameEvents and it's just a matter
handling it in a different place. If it is too much trouble and others are
blocked, go ahead and land :)



> and the
>> DIChromeFrameEvents is more useful for public interface users; e.g. I
>> don't think we should let users of the public interface dictate
>> whether the dialog is shown. Happy to discuss further by phone.
>>
>
> So the ActiveDoc and Privileged interface users are binary embedders and
> it's fine to have them say over what needs to happen. For Activex/NPAPI
> within a page, it's better if the developer knows that GCF is not going to
> function due to version mismatch. In that case she might want to display a
> better message on the page instead of the message box?
>
> In any case, if others are blocked on this, feel free to land it and
> address it in the subsequent CLs.
>
>
>> Cheers,
>> Jói
>>
>>
>> 2010/11/5 Amit Joshi <amit@chromium.org>:
>> >
>> > On Fri, Nov 5, 2010 at 12:52 PM, Jói Sigurðsson <joi@chromium.org>
>> wrote:
>> >>
>> >> > I think that makes sense and
>> essentially ShouldShowVersionMismatchDialog
>> >> > is
>> >> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents
>> for
>> >> > this notification allows other customers including Javascript and
>> tests
>> >> > to handle this uniformly.
>> >>
>> >> I'm not sure whether you're asking me to change it before checking in
>> >> or not, please let me know. I am willing change it if you think you
>> >> want this in the public interface, it's a fair amount of extra work
>> >> but the if it's the right thing, then it's the right thing.
>> >
>> > IMO, that's the right thing since it's of a generic  value. Hopefully it
>> > shouldn't be a lot of extra work (follow how "CloseWindow" event is
>> > implemented).
>> >>
>> >> Cheers,
>> >> Jói
>> >>
>> >>
>> >>
>> >> On Fri, Nov 5, 2010 at 1:47 PM, Amit Joshi <amit@chromium.org> wrote:
>> >> >
>> >> >
>> >> > On Fri, Nov 5, 2010 at 10:29 AM, <joi@chromium.org> wrote:
>> >> >>
>> >> >> Thanks Amit, see responses below.
>> >> >>
>> >> >>
>> >> >> http://codereview.chromium.org/4563001/diff/1/14
>> >> >> File chrome_frame/chrome_frame_activex.cc (right):
>> >> >>
>> >> >> http://codereview.chromium.org/4563001/diff/1/14#newcode276
>> >> >> chrome_frame/chrome_frame_activex.cc:276:
>> >> >> THREAD_SAFE_UMA_HISTOGRAM_COUNTS(
>> >> >> On 2010/11/05 17:23:04, amit wrote:
>> >> >>>
>> >> >>> nit: we should record UMA version mismatch irrespective of
>> >> >>> ShouldShowVersionMismatchDialog
>> >> >>
>> >> >> The count name is "VersionMismatchDisplayed" (note the last bit)
>> which
>> >> >> is why I didn't increment it (that and the previous code didn't
>> >> >> either).
>> >> >> Let me know which is the right thing to do.
>> >> >
>> >> > Robert can have a final say here, but I think the intent here is to
>> >> > measure
>> >> > how many times the version mismatch is happening so that we can see
>> >> > effectiveness of newer versioning scheme over the old one.  Maybe the
>> >> > right
>> >> > thing to do is change 'VersionMismatchDisplayed' to just
>> >> > VersionMismatch.
>> >> > But, I guess that's beyond the scope of this CL.
>> >> >
>> >> >>
>> >> >> http://codereview.chromium.org/4563001/diff/1/17
>> >> >> File chrome_frame/chrome_tab.idl (right):
>> >> >>
>> >> >> http://codereview.chromium.org/4563001/diff/1/17#newcode119
>> >> >> chrome_frame/chrome_tab.idl:119: HRESULT
>> >> >> ShouldShowVersionMismatchDialog();
>> >> >> On 2010/11/05 17:23:04, amit wrote:
>> >> >>>
>> >> >>> Would it be better to give this control to the privileged client?
>> So
>> >> >>
>> >> >> instead of
>> >> >>>
>> >> >>> ShouldShowVersionMismatchDialog(), we expose VersionMismatch Event
>> >> >>
>> >> >> (through
>> >> >>>
>> >> >>> DIChromeFrameEvents or adding OnVersionMismatch here if that's too
>> >> >>
>> >> >> much
>> >> >>>
>> >> >>> trouble). The plugin (Activex, ActiveDoc or NPAPI) will show the
>> >> >>
>> >> >> dialog only if
>> >> >>>
>> >> >>> the event is not handled successfully.
>> >> >>
>> >> >> I think the current approach gives maximum flexibility as well as
>> reuse
>> >> >> of the existing dialog code.
>> >> >>
>> >> >> If you always want to take over showing a dialog or other UI, you
>> >> >> always
>> >> >> respond S_FALSE and do your own thing. If you want to reuse the
>> current
>> >> >> dialog, you respond S_OK in the cases where you want it.
>> >> >
>> >> > I think that makes sense and
>> essentially ShouldShowVersionMismatchDialog
>> >> > is
>> >> > very close to OnVersionMismatch.  However, using DIChromeFrameEvents
>> for
>> >> > this notification allows other customers including Javascript and
>> tests
>> >> > to handle this uniformly.
>> >> >>
>> >> >>
>> >> >> http://codereview.chromium.org/4563001/show
>> >> >
>> >> >
>> >
>> >
>>
>
>

[Jói, 2010-11-06 20:16:35]

LGTM

The new version I uploaded adds some build-related goodness (building
chrome_frame.idl once instead of multiple times) and passes a clobber build on
the trybots, so given Amit's green light to land as is I think I'll go ahead and
do that. We can talk on Monday about how best to expose this through the public
interface.

Cheers,
Jói

http://codereview.chromium.org/4563001/diff/1/11
File ceee/testing/utils/mock_com.h (right):

http://codereview.chromium.org/4563001/diff/1/11#newcode29
ceee/testing/utils/mock_com.h:29: #include
"ceee/testing/utils/mock_ioleclientsite.gen"
On 2010/11/05 17:22:26, robertshield wrote:
> On 2010/11/05 17:02:01, Jói wrote:
> > On 2010/11/05 13:45:29, robertshield wrote:
> > > This is cool, but since the .gen file isn't being udpated automatically,
> could
> > > we just paste the generated mock method definitions here and avoid the
> inline
> > > #include?
> > 
> > We could, I'm keeping consistent with this file.
> > 
> > I'd like to check in as-is if it's OK. I was thinking of revamping this
whole
> > mock generation business, now that there is an established pattern of using
> it,
> > so that the tool outputs full header files with an IFooMockImpl and a
MockIFoo
> > object as per the pattern you see in this file.
> > 
> > This file could then limit its definitions to objects that use more than one
> > MockImpl, such as MockIWebBrowser2.
> 
> Ok, the described revamp sounds like a Good Thing to me. I'm fine with this as
> is, I guess I was mainly musing :-)

Logged as crbug.com/62117

http://codereview.chromium.org/4563001/diff/1/13
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/4563001/diff/1/13#newcode97
chrome_frame/chrome_frame.gyp:97:
'<(SHARED_INTERMEDIATE_DIR)/mock_ichromeframeprivileged.gen',
On 2010/11/05 17:22:26, robertshield wrote:
> On 2010/11/05 17:02:01, Jói wrote:
> > On 2010/11/05 13:45:29, robertshield wrote:
> > > why is this both a source and an output? won't that cause this action to
run
> > on
> > > every build whether it needs to or not?
> > 
> > I think this may be wrong, I copied the idea from generation steps in
> > ceee/testing/utils. My sleep-deprived brain at the time told me this might
be
> > weird, but I justified it in terms of "well, if anybody modifies the output
> > file, you'd want to regenerate it, right?" :-)
> > 
> > The try bot also complained on something related to this rule so I'll take a
> > better look.
> 
> Ok, let me know what you find out, I'd be surprised if a target with the same
> source and output file was the right thing to do :-)

I removed it and a clobber build on the trybots seems fine, so I will leave it
out. Siggi mentioned there might have been at some point some problem with
rebuilding things that depended on generated code, and that specifying as a
source would work around that, but until we see such a problem I'll leave it out
- that was ages ago.

http://codereview.chromium.org/4563001/diff/1/13#newcode105
chrome_frame/chrome_frame.gyp:105: '../ceee/testing/utils/com_mock.py',
On 2010/11/05 17:22:26, robertshield wrote:
> On 2010/11/05 17:02:01, Jói wrote:
> > On 2010/11/05 13:45:29, robertshield wrote:
> > > Along the lines of the previous comment, scripts that are run as actions
are
> > > supposed to, once they've computed their output, check it against any
output
> > > file already present in the build directory. 
> > > 
> > > If the output file is already binary-identical to what would be written
then
> > no
> > > writes should take place to avoid triggering a rebuild of any other
targets
> > that
> > > depend on the output. This would require changes to com_mock.py but is
> > > definitely worth doing.
> > 
> > I will submit that this should be a GYP feature, it would be easy to
> generalize
> > for any code generation steps.
> > 
> 
> This is true. 
> 
> > That being said, I'll put a TODO in com_mock.py to fix this at the same time
I
> > revamp the generation of code as mentioned in my other comment response.
> 
> sgtm.

Done.

[Jói, 2010-11-06 20:22:55]

Heh not sure why I just LGTMed my own change :-)

Anyway, looks like Siggi landed some stuff earlier today that I need
to resolve against this change, so there will be another try bot cycle
and then I'll commit unless I hear objections.

Cheers,
Jói


On Sat, Nov 6, 2010 at 4:16 PM,  <joi@chromium.org> wrote:
> LGTM
>
> The new version I uploaded adds some build-related goodness (building
> chrome_frame.idl once instead of multiple times) and passes a clobber build
> on
> the trybots, so given Amit's green light to land as is I think I'll go ahead
> and
> do that. We can talk on Monday about how best to expose this through the
> public
> interface.
>
> Cheers,
> Jói
>
>
>
> http://codereview.chromium.org/4563001/diff/1/11
> File ceee/testing/utils/mock_com.h (right):
>
> http://codereview.chromium.org/4563001/diff/1/11#newcode29
> ceee/testing/utils/mock_com.h:29: #include
> "ceee/testing/utils/mock_ioleclientsite.gen"
> On 2010/11/05 17:22:26, robertshield wrote:
>>
>> On 2010/11/05 17:02:01, Jói wrote:
>> > On 2010/11/05 13:45:29, robertshield wrote:
>> > > This is cool, but since the .gen file isn't being udpated
>
> automatically,
>>
>> could
>> > > we just paste the generated mock method definitions here and avoid
>
> the
>>
>> inline
>> > > #include?
>> >
>> > We could, I'm keeping consistent with this file.
>> >
>> > I'd like to check in as-is if it's OK. I was thinking of revamping
>
> this whole
>>
>> > mock generation business, now that there is an established pattern
>
> of using
>>
>> it,
>> > so that the tool outputs full header files with an IFooMockImpl and
>
> a MockIFoo
>>
>> > object as per the pattern you see in this file.
>> >
>> > This file could then limit its definitions to objects that use more
>
> than one
>>
>> > MockImpl, such as MockIWebBrowser2.
>
>> Ok, the described revamp sounds like a Good Thing to me. I'm fine with
>
> this as
>>
>> is, I guess I was mainly musing :-)
>
> Logged as crbug.com/62117
>
> http://codereview.chromium.org/4563001/diff/1/13
> File chrome_frame/chrome_frame.gyp (right):
>
> http://codereview.chromium.org/4563001/diff/1/13#newcode97
> chrome_frame/chrome_frame.gyp:97:
> '<(SHARED_INTERMEDIATE_DIR)/mock_ichromeframeprivileged.gen',
> On 2010/11/05 17:22:26, robertshield wrote:
>>
>> On 2010/11/05 17:02:01, Jói wrote:
>> > On 2010/11/05 13:45:29, robertshield wrote:
>> > > why is this both a source and an output? won't that cause this
>
> action to run
>>
>> > on
>> > > every build whether it needs to or not?
>> >
>> > I think this may be wrong, I copied the idea from generation steps
>
> in
>>
>> > ceee/testing/utils. My sleep-deprived brain at the time told me this
>
> might be
>>
>> > weird, but I justified it in terms of "well, if anybody modifies the
>
> output
>>
>> > file, you'd want to regenerate it, right?" :-)
>> >
>> > The try bot also complained on something related to this rule so
>
> I'll take a
>>
>> > better look.
>
>> Ok, let me know what you find out, I'd be surprised if a target with
>
> the same
>>
>> source and output file was the right thing to do :-)
>
> I removed it and a clobber build on the trybots seems fine, so I will
> leave it out. Siggi mentioned there might have been at some point some
> problem with rebuilding things that depended on generated code, and that
> specifying as a source would work around that, but until we see such a
> problem I'll leave it out - that was ages ago.
>
> http://codereview.chromium.org/4563001/diff/1/13#newcode105
> chrome_frame/chrome_frame.gyp:105: '../ceee/testing/utils/com_mock.py',
> On 2010/11/05 17:22:26, robertshield wrote:
>>
>> On 2010/11/05 17:02:01, Jói wrote:
>> > On 2010/11/05 13:45:29, robertshield wrote:
>> > > Along the lines of the previous comment, scripts that are run as
>
> actions are
>>
>> > > supposed to, once they've computed their output, check it against
>
> any output
>>
>> > > file already present in the build directory.
>> > >
>> > > If the output file is already binary-identical to what would be
>
> written then
>>
>> > no
>> > > writes should take place to avoid triggering a rebuild of any
>
> other targets
>>
>> > that
>> > > depend on the output. This would require changes to com_mock.py
>
> but is
>>
>> > > definitely worth doing.
>> >
>> > I will submit that this should be a GYP feature, it would be easy to
>> generalize
>> > for any code generation steps.
>> >
>
>> This is true.
>
>> > That being said, I'll put a TODO in com_mock.py to fix this at the
>
> same time I
>>
>> > revamp the generation of code as mentioned in my other comment
>
> response.
>
>> sgtm.
>
> Done.
>
> http://codereview.chromium.org/4563001/show
>

[Jói, 2010-11-06 20:32:22]

Scratch that, I need a review of the new chrome_frame.gyp changes
before I land, and it's the weekend - will go do something less OCD :)

2010/11/6 Jói Sigurðsson <joi@chromium.org>:
> Heh not sure why I just LGTMed my own change :-)
>
> Anyway, looks like Siggi landed some stuff earlier today that I need
> to resolve against this change, so there will be another try bot cycle
> and then I'll commit unless I hear objections.
>
> Cheers,
> Jói
>
>
> On Sat, Nov 6, 2010 at 4:16 PM,  <joi@chromium.org> wrote:
>> LGTM
>>
>> The new version I uploaded adds some build-related goodness (building
>> chrome_frame.idl once instead of multiple times) and passes a clobber build
>> on
>> the trybots, so given Amit's green light to land as is I think I'll go ahead
>> and
>> do that. We can talk on Monday about how best to expose this through the
>> public
>> interface.
>>
>> Cheers,
>> Jói
>>
>>
>>
>> http://codereview.chromium.org/4563001/diff/1/11
>> File ceee/testing/utils/mock_com.h (right):
>>
>> http://codereview.chromium.org/4563001/diff/1/11#newcode29
>> ceee/testing/utils/mock_com.h:29: #include
>> "ceee/testing/utils/mock_ioleclientsite.gen"
>> On 2010/11/05 17:22:26, robertshield wrote:
>>>
>>> On 2010/11/05 17:02:01, Jói wrote:
>>> > On 2010/11/05 13:45:29, robertshield wrote:
>>> > > This is cool, but since the .gen file isn't being udpated
>>
>> automatically,
>>>
>>> could
>>> > > we just paste the generated mock method definitions here and avoid
>>
>> the
>>>
>>> inline
>>> > > #include?
>>> >
>>> > We could, I'm keeping consistent with this file.
>>> >
>>> > I'd like to check in as-is if it's OK. I was thinking of revamping
>>
>> this whole
>>>
>>> > mock generation business, now that there is an established pattern
>>
>> of using
>>>
>>> it,
>>> > so that the tool outputs full header files with an IFooMockImpl and
>>
>> a MockIFoo
>>>
>>> > object as per the pattern you see in this file.
>>> >
>>> > This file could then limit its definitions to objects that use more
>>
>> than one
>>>
>>> > MockImpl, such as MockIWebBrowser2.
>>
>>> Ok, the described revamp sounds like a Good Thing to me. I'm fine with
>>
>> this as
>>>
>>> is, I guess I was mainly musing :-)
>>
>> Logged as crbug.com/62117
>>
>> http://codereview.chromium.org/4563001/diff/1/13
>> File chrome_frame/chrome_frame.gyp (right):
>>
>> http://codereview.chromium.org/4563001/diff/1/13#newcode97
>> chrome_frame/chrome_frame.gyp:97:
>> '<(SHARED_INTERMEDIATE_DIR)/mock_ichromeframeprivileged.gen',
>> On 2010/11/05 17:22:26, robertshield wrote:
>>>
>>> On 2010/11/05 17:02:01, Jói wrote:
>>> > On 2010/11/05 13:45:29, robertshield wrote:
>>> > > why is this both a source and an output? won't that cause this
>>
>> action to run
>>>
>>> > on
>>> > > every build whether it needs to or not?
>>> >
>>> > I think this may be wrong, I copied the idea from generation steps
>>
>> in
>>>
>>> > ceee/testing/utils. My sleep-deprived brain at the time told me this
>>
>> might be
>>>
>>> > weird, but I justified it in terms of "well, if anybody modifies the
>>
>> output
>>>
>>> > file, you'd want to regenerate it, right?" :-)
>>> >
>>> > The try bot also complained on something related to this rule so
>>
>> I'll take a
>>>
>>> > better look.
>>
>>> Ok, let me know what you find out, I'd be surprised if a target with
>>
>> the same
>>>
>>> source and output file was the right thing to do :-)
>>
>> I removed it and a clobber build on the trybots seems fine, so I will
>> leave it out. Siggi mentioned there might have been at some point some
>> problem with rebuilding things that depended on generated code, and that
>> specifying as a source would work around that, but until we see such a
>> problem I'll leave it out - that was ages ago.
>>
>> http://codereview.chromium.org/4563001/diff/1/13#newcode105
>> chrome_frame/chrome_frame.gyp:105: '../ceee/testing/utils/com_mock.py',
>> On 2010/11/05 17:22:26, robertshield wrote:
>>>
>>> On 2010/11/05 17:02:01, Jói wrote:
>>> > On 2010/11/05 13:45:29, robertshield wrote:
>>> > > Along the lines of the previous comment, scripts that are run as
>>
>> actions are
>>>
>>> > > supposed to, once they've computed their output, check it against
>>
>> any output
>>>
>>> > > file already present in the build directory.
>>> > >
>>> > > If the output file is already binary-identical to what would be
>>
>> written then
>>>
>>> > no
>>> > > writes should take place to avoid triggering a rebuild of any
>>
>> other targets
>>>
>>> > that
>>> > > depend on the output. This would require changes to com_mock.py
>>
>> but is
>>>
>>> > > definitely worth doing.
>>> >
>>> > I will submit that this should be a GYP feature, it would be easy to
>>> generalize
>>> > for any code generation steps.
>>> >
>>
>>> This is true.
>>
>>> > That being said, I'll put a TODO in com_mock.py to fix this at the
>>
>> same time I
>>>
>>> > revamp the generation of code as mentioned in my other comment
>>
>> response.
>>
>>> sgtm.
>>
>> Done.
>>
>> http://codereview.chromium.org/4563001/show
>>
>

[robertshield, 2010-11-08 20:36:04]

chrome_frame.gyp changes LGTM

On 2010/11/06 20:32:22, Jói wrote:
> Scratch that, I need a review of the new chrome_frame.gyp changes
> before I land, and it's the weekend - will go do something less OCD :)
> 
> 2010/11/6 Jói Sigurðsson <joi@chromium.org>:
> > Heh not sure why I just LGTMed my own change :-)
> >
> > Anyway, looks like Siggi landed some stuff earlier today that I need
> > to resolve against this change, so there will be another try bot cycle
> > and then I'll commit unless I hear objections.
> >
> > Cheers,
> > Jói
> >
> >
> > On Sat, Nov 6, 2010 at 4:16 PM, mailto: <joi@chromium.org> wrote:
> >> LGTM
> >>
> >> The new version I uploaded adds some build-related goodness (building
> >> chrome_frame.idl once instead of multiple times) and passes a clobber build
> >> on
> >> the trybots, so given Amit's green light to land as is I think I'll go
ahead
> >> and
> >> do that. We can talk on Monday about how best to expose this through the
> >> public
> >> interface.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >>
> >> http://codereview.chromium.org/4563001/diff/1/11
> >> File ceee/testing/utils/mock_com.h (right):
> >>
> >> http://codereview.chromium.org/4563001/diff/1/11#newcode29
> >> ceee/testing/utils/mock_com.h:29: #include
> >> "ceee/testing/utils/mock_ioleclientsite.gen"
> >> On 2010/11/05 17:22:26, robertshield wrote:
> >>>
> >>> On 2010/11/05 17:02:01, Jói wrote:
> >>> > On 2010/11/05 13:45:29, robertshield wrote:
> >>> > > This is cool, but since the .gen file isn't being udpated
> >>
> >> automatically,
> >>>
> >>> could
> >>> > > we just paste the generated mock method definitions here and avoid
> >>
> >> the
> >>>
> >>> inline
> >>> > > #include?
> >>> >
> >>> > We could, I'm keeping consistent with this file.
> >>> >
> >>> > I'd like to check in as-is if it's OK. I was thinking of revamping
> >>
> >> this whole
> >>>
> >>> > mock generation business, now that there is an established pattern
> >>
> >> of using
> >>>
> >>> it,
> >>> > so that the tool outputs full header files with an IFooMockImpl and
> >>
> >> a MockIFoo
> >>>
> >>> > object as per the pattern you see in this file.
> >>> >
> >>> > This file could then limit its definitions to objects that use more
> >>
> >> than one
> >>>
> >>> > MockImpl, such as MockIWebBrowser2.
> >>
> >>> Ok, the described revamp sounds like a Good Thing to me. I'm fine with
> >>
> >> this as
> >>>
> >>> is, I guess I was mainly musing :-)
> >>
> >> Logged as crbug.com/62117
> >>
> >> http://codereview.chromium.org/4563001/diff/1/13
> >> File chrome_frame/chrome_frame.gyp (right):
> >>
> >> http://codereview.chromium.org/4563001/diff/1/13#newcode97
> >> chrome_frame/chrome_frame.gyp:97:
> >> '<(SHARED_INTERMEDIATE_DIR)/mock_ichromeframeprivileged.gen',
> >> On 2010/11/05 17:22:26, robertshield wrote:
> >>>
> >>> On 2010/11/05 17:02:01, Jói wrote:
> >>> > On 2010/11/05 13:45:29, robertshield wrote:
> >>> > > why is this both a source and an output? won't that cause this
> >>
> >> action to run
> >>>
> >>> > on
> >>> > > every build whether it needs to or not?
> >>> >
> >>> > I think this may be wrong, I copied the idea from generation steps
> >>
> >> in
> >>>
> >>> > ceee/testing/utils. My sleep-deprived brain at the time told me this
> >>
> >> might be
> >>>
> >>> > weird, but I justified it in terms of "well, if anybody modifies the
> >>
> >> output
> >>>
> >>> > file, you'd want to regenerate it, right?" :-)
> >>> >
> >>> > The try bot also complained on something related to this rule so
> >>
> >> I'll take a
> >>>
> >>> > better look.
> >>
> >>> Ok, let me know what you find out, I'd be surprised if a target with
> >>
> >> the same
> >>>
> >>> source and output file was the right thing to do :-)
> >>
> >> I removed it and a clobber build on the trybots seems fine, so I will
> >> leave it out. Siggi mentioned there might have been at some point some
> >> problem with rebuilding things that depended on generated code, and that
> >> specifying as a source would work around that, but until we see such a
> >> problem I'll leave it out - that was ages ago.
> >>
> >> http://codereview.chromium.org/4563001/diff/1/13#newcode105
> >> chrome_frame/chrome_frame.gyp:105: '../ceee/testing/utils/com_mock.py',
> >> On 2010/11/05 17:22:26, robertshield wrote:
> >>>
> >>> On 2010/11/05 17:02:01, Jói wrote:
> >>> > On 2010/11/05 13:45:29, robertshield wrote:
> >>> > > Along the lines of the previous comment, scripts that are run as
> >>
> >> actions are
> >>>
> >>> > > supposed to, once they've computed their output, check it against
> >>
> >> any output
> >>>
> >>> > > file already present in the build directory.
> >>> > >
> >>> > > If the output file is already binary-identical to what would be
> >>
> >> written then
> >>>
> >>> > no
> >>> > > writes should take place to avoid triggering a rebuild of any
> >>
> >> other targets
> >>>
> >>> > that
> >>> > > depend on the output. This would require changes to com_mock.py
> >>
> >> but is
> >>>
> >>> > > definitely worth doing.
> >>> >
> >>> > I will submit that this should be a GYP feature, it would be easy to
> >>> generalize
> >>> > for any code generation steps.
> >>> >
> >>
> >>> This is true.
> >>
> >>> > That being said, I'll put a TODO in com_mock.py to fix this at the
> >>
> >> same time I
> >>>
> >>> > revamp the generation of code as mentioned in my other comment
> >>
> >> response.
> >>
> >>> sgtm.
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/4563001/show
> >>
> >
>