Switch renderer in Moniker patch...

Switch renderer in Moniker patch

Step one of the changes. Inspect data and cause a switch
in the moniker patch. 



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

[tfarina (gmail-do not use), 2010-04-02 21:39:31]

http://codereview.chromium.org/1589013/diff/1/2
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/1/2#newcode30
chrome_frame/urlmon_bind_status_callback.cc:30: }
nit: // namespace

http://codereview.chromium.org/1589013/diff/1/2#newcode581
chrome_frame/urlmon_bind_status_callback.cc:581: 
nit: remove extra blank lines?

http://codereview.chromium.org/1589013/diff/1/3
File chrome_frame/urlmon_bind_status_callback.h (right):

http://codereview.chromium.org/1589013/diff/1/3#newcode96
chrome_frame/urlmon_bind_status_callback.h:96: public:
nit: indent one space. And the others 2 spaces.

http://codereview.chromium.org/1589013/diff/1/3#newcode274
chrome_frame/urlmon_bind_status_callback.h:274: HRESULT
StartSniffing(DataSniffDelegate* delegate, IMoniker* moniker, IBindCtx*
bind_ctx);
nit: line > 80.

http://codereview.chromium.org/1589013/diff/1/3#newcode291
chrome_frame/urlmon_bind_status_callback.h:291: 
nit: remove extra blank lines?

[stoyan, 2010-04-02 23:54:16]

http://codereview.chromium.org/1589013/diff/11001/12001
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/11001/12001#newcode17
chrome_frame/urlmon_bind_status_callback.cc:17: HRESULT CloneMoniker(IBindCtx*
bind_ctx, IMoniker* moniker, IMoniker** out) {
Who calls this function?
BTW I think NULL bind_ctx will do just fine in IMoniker::GetDisplayName().

http://codereview.chromium.org/1589013/diff/11001/12002
File chrome_frame/urlmon_bind_status_callback.h (right):

http://codereview.chromium.org/1589013/diff/11001/12002#newcode148
chrome_frame/urlmon_bind_status_callback.h:148: class BSCBBase : public BSCBImpl
{
Seems this class is keeping some cached data and also sniffs for metatag. Why it
derives from callback thingy?
Wouldn't be much cleaner if this is a separate class that is aggregated by the
real callback?
Passing appropriate arguments to ReadIntoCache and DrainCache should avoid this
awful dependency with the base class.

Inheritance is the new goto. :)

[tommi (sloooow) - chröme, 2010-04-06 15:04:52]

I haven't looked at the change in detail but I notice you added use of
net::HttpUtil::AssembleRawHeaders.
I'm worried that this could be a regression since AssembleRawHeaders
replaces \r\n with \0 and the subsequent header enumeration code doesn't
work anymore.

On Fri, Apr 2, 2010 at 9:39 PM, <thiago.farina@gmail.com> wrote:

>
> http://codereview.chromium.org/1589013/diff/1/2
> File chrome_frame/urlmon_bind_status_callback.cc (right):
>
> http://codereview.chromium.org/1589013/diff/1/2#newcode30
> chrome_frame/urlmon_bind_status_callback.cc:30: }
> nit: // namespace
>
> http://codereview.chromium.org/1589013/diff/1/2#newcode581
> chrome_frame/urlmon_bind_status_callback.cc:581:
> nit: remove extra blank lines?
>
> http://codereview.chromium.org/1589013/diff/1/3
> File chrome_frame/urlmon_bind_status_callback.h (right):
>
> http://codereview.chromium.org/1589013/diff/1/3#newcode96
> chrome_frame/urlmon_bind_status_callback.h:96: public:
> nit: indent one space. And the others 2 spaces.
>
> http://codereview.chromium.org/1589013/diff/1/3#newcode274
> chrome_frame/urlmon_bind_status_callback.h:274: HRESULT
> StartSniffing(DataSniffDelegate* delegate, IMoniker* moniker, IBindCtx*
> bind_ctx);
> nit: line > 80.
>
> http://codereview.chromium.org/1589013/diff/1/3#newcode291
> chrome_frame/urlmon_bind_status_callback.h:291:
> nit: remove extra blank lines?
>
> http://codereview.chromium.org/1589013
>

[amit, 2010-04-07 01:59:21]

Changes done, new tests added. Please continue with the review.

[ananta, 2010-04-07 18:56:14]

Looks great.

http://codereview.chromium.org/1589013/diff/4002/29015
File chrome_frame/bho.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29015#newcode323
chrome_frame/bho.cc:323: DCHECK(patch_method == PATCH_METHOD_IBROWSER ||
Should we remove PATCH_METHOD_IBROWSER and the OnHttpEquiv patch. This ensures
that there is no confusion while debugging .

http://codereview.chromium.org/1589013/diff/4002/29014
File chrome_frame/bho.h (right):

http://codereview.chromium.org/1589013/diff/4002/29014#newcode30
chrome_frame/bho.h:30: enum State { UNKNOWN, PATCH_IBROWSER, PATCH_PROTOCOL,
PATCH_MONIKER };
Get rid of PATCH_IBROWSER?

http://codereview.chromium.org/1589013/diff/4002/29024
File chrome_frame/bind_status_callback_impl.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29024#newcode33
chrome_frame/bind_status_callback_impl.cc:33: delegate_ = original;
If we don't have a delegate then shouldn't we just bail?. Perhaps the method
should have a return value indicating success or failure?

http://codereview.chromium.org/1589013/diff/4002/29024#newcode38
chrome_frame/bind_status_callback_impl.cc:38: hr =
::RegisterBindStatusCallback(bind_ctx, this, delegate_.Receive(), 0);
If there is no delegate_ here then what should this object do?

http://codereview.chromium.org/1589013/diff/4002/29025
File chrome_frame/bind_status_callback_impl.h (right):

http://codereview.chromium.org/1589013/diff/4002/29025#newcode16
chrome_frame/bind_status_callback_impl.h:16: class BSCBImpl
Some comments here about what this class does. Perhaps the name should
BindStatusCallbackImpl?

http://codereview.chromium.org/1589013/diff/4002/29023
File chrome_frame/stream_impl.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29023#newcode13
chrome_frame/stream_impl.cc:13: 
Nit: Remove extra newline.

http://codereview.chromium.org/1589013/diff/4002/29019
File chrome_frame/test/urlmon_moniker_integration_test.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29019#newcode263
chrome_frame/test/urlmon_moniker_integration_test.cc:263: /*
Either remove these tests or disable them with a bug.

http://codereview.chromium.org/1589013/diff/4002/29021
File chrome_frame/test/urlmon_moniker_unittest.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29021#newcode25
chrome_frame/test/urlmon_moniker_unittest.cc:25:
.Append(FILE_PATH_LITERAL("test"))
Perhaps chrome_frame\test\data?

http://codereview.chromium.org/1589013/diff/4002/29021#newcode40
chrome_frame/test/urlmon_moniker_unittest.cc:40: if (FAILED(hr)) {
Perhaps you can spray EXPECT_ calls here to flag errors instead of NOTREACHED
and DCHECKS

http://codereview.chromium.org/1589013/diff/4002/29021#newcode64
chrome_frame/test/urlmon_moniker_unittest.cc:64: TEST_F(MonikerPatchTest,
CacheStream) {
Nice tests :)

http://codereview.chromium.org/1589013/diff/4002/29002
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29002#newcode139
chrome_frame/urlmon_bind_status_callback.cc:139: if (0
/*IsOptInUrl(url_.c_str())*/) {
I guess this was only for debugging. Please fix this.

http://codereview.chromium.org/1589013/diff/4002/29002#newcode171
chrome_frame/urlmon_bind_status_callback.cc:171: NOTREACHED() << __FUNCTION__ <<
me() << "AttachToBind error: " << hr;
Bail here?

http://codereview.chromium.org/1589013/diff/4002/29002#newcode208
chrome_frame/urlmon_bind_status_callback.cc:208: if ((stgmed->tymed !=
TYMED_ISTREAM) || !stgmed->pstm) {
Add a check for stgmed?

http://codereview.chromium.org/1589013/diff/4002/29002#newcode213
chrome_frame/urlmon_bind_status_callback.cc:213: HRESULT hr = S_OK;
It would be very helpful if you could add some comments about the overall logic
in this function.

http://codereview.chromium.org/1589013/diff/4002/29002#newcode279
chrome_frame/urlmon_bind_status_callback.cc:279:
bind_ctx_->RegisterObjectParam(L"_CHROMEFRAME_PRECREATE_",
Define this statically in this file.

http://codereview.chromium.org/1589013/diff/4002/29003
File chrome_frame/urlmon_bind_status_callback.h (right):

http://codereview.chromium.org/1589013/diff/4002/29003#newcode46
chrome_frame/urlmon_bind_status_callback.h:46: 
Nit: Remove extra line

http://codereview.chromium.org/1589013/diff/4002/29004
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29004#newcode63
chrome_frame/urlmon_moniker.cc:63: hr = ::CreateBindCtx(0,
bind_context.Receive());
Perhaps we should use the urlmon version of the bind context here. That
unfortunately requires a dummy callback to be registered on it though. We don't
want to go on a wild goose chase in the future.

http://codereview.chromium.org/1589013/diff/4002/29004#newcode185
chrome_frame/urlmon_moniker.cc:185: hr =
bind_ctx->GetObjectParam(L"_CHROMEFRAME_PRECREATE_", cf_doc.Receive());
Define this constant in a shared place.

http://codereview.chromium.org/1589013/diff/4002/29004#newcode191
chrome_frame/urlmon_moniker.cc:191: DCHECK(SUCCEEDED(hr));
Is there any point in continuing here if we fail to query IPersistMoniker?

http://codereview.chromium.org/1589013/diff/4002/29010
File chrome_frame/urlmon_moniker.h (right):

http://codereview.chromium.org/1589013/diff/4002/29010#newcode49
chrome_frame/urlmon_moniker.h:49: //   renderer chrome. It then registers a
special bind context param and
is chrome?

http://codereview.chromium.org/1589013/diff/4002/29009
File chrome_frame/urlmon_moniker_base.h (left):

http://codereview.chromium.org/1589013/diff/4002/29009#oldcode1
chrome_frame/urlmon_moniker_base.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
Remove this file

[amit, 2010-04-08 02:42:47]

The experiment of initially returning CF_NULL as a mimetype and then switching
does not work. mshtml will make up its mind about the mime type in the very
first OnDataAvailable call. 

Simplified existing logic in OnDataAvailable to read the full contents of the
stream as per stoyan's recommendation.

New patch up, please continue reviewing.

http://codereview.chromium.org/1589013/diff/4002/29015
File chrome_frame/bho.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29015#newcode323
chrome_frame/bho.cc:323: DCHECK(patch_method == PATCH_METHOD_IBROWSER ||
On 2010/04/07 18:56:14, ananta wrote:
> Should we remove PATCH_METHOD_IBROWSER and the OnHttpEquiv patch. This ensures
> that there is no confusion while debugging .

True, but it will come in handy to test in the short term. In the longer term I
think we should remove this and the protocol patch as well.

http://codereview.chromium.org/1589013/diff/4002/29024
File chrome_frame/bind_status_callback_impl.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29024#newcode33
chrome_frame/bind_status_callback_impl.cc:33: delegate_ = original;
On 2010/04/07 18:56:14, ananta wrote:
> If we don't have a delegate then shouldn't we just bail?. Perhaps the method
> should have a return value indicating success or failure?

This is a generic base class and works even if there's no delegate. The derived
class BSCBStorageBind checks for the existence of the delegate after
AttachToBind

http://codereview.chromium.org/1589013/diff/4002/29025
File chrome_frame/bind_status_callback_impl.h (right):

http://codereview.chromium.org/1589013/diff/4002/29025#newcode16
chrome_frame/bind_status_callback_impl.h:16: class BSCBImpl
On 2010/04/07 18:56:14, ananta wrote:
> Some comments here about what this class does. Perhaps the name should
> BindStatusCallbackImpl?
> 

Done.

http://codereview.chromium.org/1589013/diff/4002/29023
File chrome_frame/stream_impl.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29023#newcode13
chrome_frame/stream_impl.cc:13: 
On 2010/04/07 18:56:14, ananta wrote:
> Nit: Remove extra newline.

Done.

http://codereview.chromium.org/1589013/diff/4002/29021
File chrome_frame/test/urlmon_moniker_unittest.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29021#newcode25
chrome_frame/test/urlmon_moniker_unittest.cc:25:
.Append(FILE_PATH_LITERAL("test"))
On 2010/04/07 18:56:14, ananta wrote:
> Perhaps chrome_frame\test\data?

Well, '\' may not be separator on all platforms, hence it's better to use Append
sequence.

http://codereview.chromium.org/1589013/diff/4002/29021#newcode40
chrome_frame/test/urlmon_moniker_unittest.cc:40: if (FAILED(hr)) {
On 2010/04/07 18:56:14, ananta wrote:
> Perhaps you can spray EXPECT_ calls here to flag errors instead of NOTREACHED
> and DCHECKS

Done.

http://codereview.chromium.org/1589013/diff/4002/29002
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29002#newcode139
chrome_frame/urlmon_bind_status_callback.cc:139: if (0
/*IsOptInUrl(url_.c_str())*/) {
On 2010/04/07 18:56:14, ananta wrote:
> I guess this was only for debugging. Please fix this.

Done.

http://codereview.chromium.org/1589013/diff/4002/29002#newcode171
chrome_frame/urlmon_bind_status_callback.cc:171: NOTREACHED() << __FUNCTION__ <<
me() << "AttachToBind error: " << hr;
On 2010/04/07 18:56:14, ananta wrote:
> Bail here?

Done.

http://codereview.chromium.org/1589013/diff/4002/29002#newcode208
chrome_frame/urlmon_bind_status_callback.cc:208: if ((stgmed->tymed !=
TYMED_ISTREAM) || !stgmed->pstm) {
On 2010/04/07 18:56:14, ananta wrote:
> Add a check for stgmed?

Good point, done

http://codereview.chromium.org/1589013/diff/4002/29002#newcode213
chrome_frame/urlmon_bind_status_callback.cc:213: HRESULT hr = S_OK;
On 2010/04/07 18:56:14, ananta wrote:
> It would be very helpful if you could add some comments about the overall
logic
> in this function.
> 

Referred urlmon_moniker.h comments here.

http://codereview.chromium.org/1589013/diff/4002/29002#newcode279
chrome_frame/urlmon_bind_status_callback.cc:279:
bind_ctx_->RegisterObjectParam(L"_CHROMEFRAME_PRECREATE_",
On 2010/04/07 18:56:14, ananta wrote:
> Define this statically in this file.

Done.

http://codereview.chromium.org/1589013/diff/4002/29004
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29004#newcode63
chrome_frame/urlmon_moniker.cc:63: hr = ::CreateBindCtx(0,
bind_context.Receive());
On 2010/04/07 18:56:14, ananta wrote:
> Perhaps we should use the urlmon version of the bind context here. That
> unfortunately requires a dummy callback to be registered on it though. We
don't
> want to go on a wild goose chase in the future.

Done.

http://codereview.chromium.org/1589013/diff/4002/29004#newcode185
chrome_frame/urlmon_moniker.cc:185: hr =
bind_ctx->GetObjectParam(L"_CHROMEFRAME_PRECREATE_", cf_doc.Receive());
On 2010/04/07 18:56:14, ananta wrote:
> Define this constant in a shared place.

Done.

http://codereview.chromium.org/1589013/diff/4002/29004#newcode191
chrome_frame/urlmon_moniker.cc:191: DCHECK(SUCCEEDED(hr));
On 2010/04/07 18:56:14, ananta wrote:
> Is there any point in continuing here if we fail to query IPersistMoniker?

Done.

http://codereview.chromium.org/1589013/diff/4002/29010
File chrome_frame/urlmon_moniker.h (right):

http://codereview.chromium.org/1589013/diff/4002/29010#newcode49
chrome_frame/urlmon_moniker.h:49: //   renderer chrome. It then registers a
special bind context param and
On 2010/04/07 18:56:14, ananta wrote:
> is chrome?

Done.

http://codereview.chromium.org/1589013/diff/4002/29009
File chrome_frame/urlmon_moniker_base.h (left):

http://codereview.chromium.org/1589013/diff/4002/29009#oldcode1
chrome_frame/urlmon_moniker_base.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2010/04/07 18:56:14, ananta wrote:
> Remove this file
It's already removed!

[tommi (sloooow) - chröme, 2010-04-08 17:05:51]

http://codereview.chromium.org/1589013/diff/4002/29024
File chrome_frame/bind_status_callback_impl.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29024#newcode46
chrome_frame/bind_status_callback_impl.cc:46: HRESULT BSCBImpl::DetachBind() {
maybe it should be called ReleaseBind

http://codereview.chromium.org/1589013/diff/4002/29024#newcode132
chrome_frame/bind_status_callback_impl.cc:132: DLOG(INFO) << __FUNCTION__ <<
StringPrintf(" tid=%i",
maybe include me() in there too? :)

http://codereview.chromium.org/1589013/diff/4002/29024#newcode151
chrome_frame/bind_status_callback_impl.cc:151: DWORD* bindf2, DWORD* reserved) {
indent

http://codereview.chromium.org/1589013/diff/4002/29024#newcode165
chrome_frame/bind_status_callback_impl.cc:165: DWORD reserved,
indent (and throughout)

http://codereview.chromium.org/1589013/diff/4002/29025
File chrome_frame/bind_status_callback_impl.h (right):

http://codereview.chromium.org/1589013/diff/4002/29025#newcode40
chrome_frame/bind_status_callback_impl.h:40: HRESULT DetachBind();
Does this detach or release?  If it detaches, shouldn't it return the detached
object?

http://codereview.chromium.org/1589013/diff/4002/29022
File chrome_frame/stream_impl.h (right):

http://codereview.chromium.org/1589013/diff/4002/29022#newcode14
chrome_frame/stream_impl.h:14: class StreamImpl : public IStream {
StreamDelegateImpl?

http://codereview.chromium.org/1589013/diff/4002/29018
File chrome_frame/test/test_with_web_server.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29018#newcode795
chrome_frame/test/test_with_web_server.cc:795:
TEST_F(ChromeFrameTestWithWebServer, FullTabModeIE_TestPostReissue) {
sync?

http://codereview.chromium.org/1589013/diff/4002/29019
File chrome_frame/test/urlmon_moniker_integration_test.cc (left):

http://codereview.chromium.org/1589013/diff/4002/29019#oldcode221
chrome_frame/test/urlmon_moniker_integration_test.cc:221:
EXPECT_FALSE(test.nav_manager().HasRequestData());
you're removing one of the things the test is meant to test. why?

http://codereview.chromium.org/1589013/diff/4002/29019#oldcode246
chrome_frame/test/urlmon_moniker_integration_test.cc:246:
EXPECT_FALSE(test.nav_manager().HasRequestData());
same here

http://codereview.chromium.org/1589013/diff/4002/29019
File chrome_frame/test/urlmon_moniker_integration_test.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29019#newcode11
chrome_frame/test/urlmon_moniker_integration_test.cc:11: //#include
"chrome_frame/urlmon_moniker.h"
remove

http://codereview.chromium.org/1589013/diff/4002/29019#newcode263
chrome_frame/test/urlmon_moniker_integration_test.cc:263: /*
re-enable tests

http://codereview.chromium.org/1589013/diff/4002/29021
File chrome_frame/test/urlmon_moniker_unittest.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29021#newcode51
chrome_frame/test/urlmon_moniker_unittest.cc:51: LARGE_INTEGER pos = {0};
can call RewindStream()

http://codereview.chromium.org/1589013/diff/4002/29021#newcode66
chrome_frame/test/urlmon_moniker_unittest.cc:66: char ret[2*sizeof(data)] = {0};
spaces around *

http://codereview.chromium.org/1589013/diff/4002/29021#newcode86
chrome_frame/test/urlmon_moniker_unittest.cc:86: ACTION_P3(ReadStream, buffer,
size, ret) {
neat - more gmock magic macros to confuse the heck out of me :)

http://codereview.chromium.org/1589013/diff/4002/29021#newcode101
chrome_frame/test/urlmon_moniker_unittest.cc:101: DWORD read_size1 = size,
read_size2 = 0xdeadbabe;
nit - make a constant for this value and use that constant wherever this babe
occurs?

http://codereview.chromium.org/1589013/diff/4002/29021#newcode105
chrome_frame/test/urlmon_moniker_unittest.cc:105:
testing::Field(&FORMATETC::cfFormat, Eq(cf)),
indent is a bit misleading

http://codereview.chromium.org/1589013/diff/4002/29021#newcode165
chrome_frame/test/urlmon_moniker_unittest.cc:165: const CLIPFORMAT cf = 0xd0d0;
-1?

http://codereview.chromium.org/1589013/diff/4002/29021#newcode169
chrome_frame/test/urlmon_moniker_unittest.cc:169: DWORD read_size1 =
data_size*2;
spaces

http://codereview.chromium.org/1589013/diff/4002/29021#newcode219
chrome_frame/test/urlmon_moniker_unittest.cc:219: DWORD read_size1 =
data_size/2; // First read is half the data.
spaces

http://codereview.chromium.org/1589013/diff/4002/29002
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29002#newcode146
chrome_frame/urlmon_bind_status_callback.cc:146: // browsers may handle this
properly, we don't and will stop scanning for
80

http://codereview.chromium.org/1589013/diff/4002/29002#newcode278
chrome_frame/urlmon_bind_status_callback.cc:278: if (cf_doc)
{}

http://codereview.chromium.org/1589013/diff/4002/29002#newcode287
chrome_frame/urlmon_bind_status_callback.cc:287: if (!size)
empty line before this one

http://codereview.chromium.org/1589013/diff/4002/29003
File chrome_frame/urlmon_bind_status_callback.h (right):

http://codereview.chromium.org/1589013/diff/4002/29003#newcode11
chrome_frame/urlmon_bind_status_callback.h:11: //#include
"chrome_frame/urlmon_moniker.h"
remove

http://codereview.chromium.org/1589013/diff/4002/29003#newcode118
chrome_frame/urlmon_bind_status_callback.h:118: 
no empty line

http://codereview.chromium.org/1589013/diff/4002/29004
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29004#newcode208
chrome_frame/urlmon_moniker.cc:208: DCHECK(iid == IID_IStream);
you can get IID_IUnknown here, so I suggest you don't change the DCHECK and
handle the IUnknown case.
IIRC it's in the case of downloads.

[stoyan, 2010-04-08 18:49:35]

http://codereview.chromium.org/1589013/diff/4004/47020
File chrome_frame/test/urlmon_moniker_integration_test.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47020#newcode115
chrome_frame/test/urlmon_moniker_integration_test.cc:115: *flags =
BINDF_PULLDATA | BINDF_NOWRITECACHE;
Hm, are you sure about BINDF_NOWRITECACHE?

http://codereview.chromium.org/1589013/diff/4004/47022
File chrome_frame/test/urlmon_moniker_unittest.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47022#newcode48
chrome_frame/test/urlmon_moniker_unittest.cc:48: EXPECT_EQ(written,
data.size());
EXPECT_EQ(data.size(), written); 
Expected value should be the first one.

http://codereview.chromium.org/1589013/diff/4004/47022#newcode56
chrome_frame/test/urlmon_moniker_unittest.cc:56: return !!(*ret);
*ret may not be initialized?

http://codereview.chromium.org/1589013/diff/4004/47022#newcode104
chrome_frame/test/urlmon_moniker_unittest.cc:104: EXPECT_CALL(mock,
OnDataAvailable(Eq(flags), Eq(size),
you don't need testing::Eq for simple values, right?

http://codereview.chromium.org/1589013/diff/4004/47022#newcode246
chrome_frame/test/urlmon_moniker_unittest.cc:246: EXPECT_EQ(std::string(),
std::string(read_buffer3));
EXPECT_STREQ?

http://codereview.chromium.org/1589013/diff/4004/47003
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47003#newcode28
chrome_frame/urlmon_bind_status_callback.cc:28: CComObject<CacheStream>*
cache_stream = NULL;
Please add a comment why this should not be CComObjectStackEx.

http://codereview.chromium.org/1589013/diff/4004/47004
File chrome_frame/urlmon_bind_status_callback.h (right):

http://codereview.chromium.org/1589013/diff/4004/47004#newcode27
chrome_frame/urlmon_bind_status_callback.h:27: static HRESULT
BSCBFeedData(IBindStatusCallback* bscb, const char* data,
This is the only public (and used) function of CacheStream class, right? You can
hide everything else (and even move this into a impl file.)

http://codereview.chromium.org/1589013/diff/4004/47005
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47005#newcode268
chrome_frame/urlmon_moniker.cc:268: if ((S_OK == hr) && callback)
add a comment, please

[stoyan, 2010-04-08 19:02:00]

http://codereview.chromium.org/1589013/diff/4004/47003
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47003#newcode239
chrome_frame/urlmon_bind_status_callback.cc:239: bool data_fully_available =
!!(bscf &
can you rename this to "force_determination" or similar for clarity? I was just
going to write comment that 
if (data_sniffer_.is_undetermined())
   return S_OK;
should fall trough if data_fully_avaialble is true.

[amit, 2010-04-08 22:52:38]

Thanks for the comments so far, new patch is up, please continue.

http://codereview.chromium.org/1589013/diff/4002/29022
File chrome_frame/stream_impl.h (right):

http://codereview.chromium.org/1589013/diff/4002/29022#newcode14
chrome_frame/stream_impl.h:14: class StreamImpl : public IStream {
On 2010/04/08 17:05:52, tommi wrote:
> StreamDelegateImpl?

Well, it could be used without a delegate. Updated the comments on top.

http://codereview.chromium.org/1589013/diff/4002/29019
File chrome_frame/test/urlmon_moniker_integration_test.cc (left):

http://codereview.chromium.org/1589013/diff/4002/29019#oldcode221
chrome_frame/test/urlmon_moniker_integration_test.cc:221:
EXPECT_FALSE(test.nav_manager().HasRequestData());
On 2010/04/08 17:05:52, tommi wrote:
> you're removing one of the things the test is meant to test. why?

Work in progress, I am going to redesign these tests. This is removed in the
short term since request data is no longer there. Please ignore this file in the
short term.

http://codereview.chromium.org/1589013/diff/4002/29021
File chrome_frame/test/urlmon_moniker_unittest.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29021#newcode51
chrome_frame/test/urlmon_moniker_unittest.cc:51: LARGE_INTEGER pos = {0};
On 2010/04/08 17:05:52, tommi wrote:
> can call RewindStream()

Done.

http://codereview.chromium.org/1589013/diff/4002/29021#newcode66
chrome_frame/test/urlmon_moniker_unittest.cc:66: char ret[2*sizeof(data)] = {0};
On 2010/04/08 17:05:52, tommi wrote:
> spaces around *

Done.

http://codereview.chromium.org/1589013/diff/4002/29021#newcode101
chrome_frame/test/urlmon_moniker_unittest.cc:101: DWORD read_size1 = size,
read_size2 = 0xdeadbabe;
On 2010/04/08 17:05:52, tommi wrote:
> nit - make a constant for this value and use that constant wherever this babe
> occurs?

Done.

http://codereview.chromium.org/1589013/diff/4002/29021#newcode105
chrome_frame/test/urlmon_moniker_unittest.cc:105:
testing::Field(&FORMATETC::cfFormat, Eq(cf)),
On 2010/04/08 17:05:52, tommi wrote:
> indent is a bit misleading
True, I wanted to indent it farther than the WillOnce below and still fit stuff
on one line so that it is readable. Would be happy to reindent if it could be
done better.

http://codereview.chromium.org/1589013/diff/4002/29021#newcode165
chrome_frame/test/urlmon_moniker_unittest.cc:165: const CLIPFORMAT cf = 0xd0d0;
On 2010/04/08 17:05:52, tommi wrote:
> -1?
Any particular reason why? Since clip formats are just atoms that start with
0xcXXX  maybe the ideal value is one of that. As the mock should just blindly
pass around clip format, I wanted something odd :)

http://codereview.chromium.org/1589013/diff/4002/29021#newcode169
chrome_frame/test/urlmon_moniker_unittest.cc:169: DWORD read_size1 =
data_size*2;
On 2010/04/08 17:05:52, tommi wrote:
> spaces

Done.

http://codereview.chromium.org/1589013/diff/4002/29021#newcode219
chrome_frame/test/urlmon_moniker_unittest.cc:219: DWORD read_size1 =
data_size/2; // First read is half the data.
On 2010/04/08 17:05:52, tommi wrote:
> spaces

Done.

http://codereview.chromium.org/1589013/diff/4002/29002
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29002#newcode146
chrome_frame/urlmon_bind_status_callback.cc:146: // browsers may handle this
properly, we don't and will stop scanning for
On 2010/04/08 17:05:52, tommi wrote:
> 80

Done.

http://codereview.chromium.org/1589013/diff/4002/29002#newcode278
chrome_frame/urlmon_bind_status_callback.cc:278: if (cf_doc)
On 2010/04/08 17:05:52, tommi wrote:
> {}

Rearranged in a separate helper as per Ananta's suggestion

http://codereview.chromium.org/1589013/diff/4002/29002#newcode287
chrome_frame/urlmon_bind_status_callback.cc:287: if (!size)
On 2010/04/08 17:05:52, tommi wrote:
> empty line before this one
Removed as per stoyan's suggestion

http://codereview.chromium.org/1589013/diff/4002/29004
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/4002/29004#newcode208
chrome_frame/urlmon_moniker.cc:208: DCHECK(iid == IID_IStream);
On 2010/04/08 17:05:52, tommi wrote:
> you can get IID_IUnknown here, so I suggest you don't change the DCHECK and
> handle the IUnknown case.
> IIRC it's in the case of downloads.

Good point, checked.

http://codereview.chromium.org/1589013/diff/4004/47020
File chrome_frame/test/urlmon_moniker_integration_test.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47020#newcode115
chrome_frame/test/urlmon_moniker_integration_test.cc:115: *flags =
BINDF_PULLDATA | BINDF_NOWRITECACHE;
On 2010/04/08 18:49:36, stoyan wrote:
> Hm, are you sure about BINDF_NOWRITECACHE?
Copied flags from urlmon_url_request.cc

http://codereview.chromium.org/1589013/diff/4004/47022
File chrome_frame/test/urlmon_moniker_unittest.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47022#newcode48
chrome_frame/test/urlmon_moniker_unittest.cc:48: EXPECT_EQ(written,
data.size());
On 2010/04/08 18:49:36, stoyan wrote:
> EXPECT_EQ(data.size(), written); 
> Expected value should be the first one.

Done.

http://codereview.chromium.org/1589013/diff/4004/47022#newcode56
chrome_frame/test/urlmon_moniker_unittest.cc:56: return !!(*ret);
On 2010/04/08 18:49:36, stoyan wrote:
> *ret may not be initialized?

Done.

http://codereview.chromium.org/1589013/diff/4004/47022#newcode104
chrome_frame/test/urlmon_moniker_unittest.cc:104: EXPECT_CALL(mock,
OnDataAvailable(Eq(flags), Eq(size),
On 2010/04/08 18:49:36, stoyan wrote:
> you don't need testing::Eq for simple values, right?

Done.

http://codereview.chromium.org/1589013/diff/4004/47022#newcode246
chrome_frame/test/urlmon_moniker_unittest.cc:246: EXPECT_EQ(std::string(),
std::string(read_buffer3));
On 2010/04/08 18:49:36, stoyan wrote:
> EXPECT_STREQ?

Done.

http://codereview.chromium.org/1589013/diff/4004/47003
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47003#newcode28
chrome_frame/urlmon_bind_status_callback.cc:28: CComObject<CacheStream>*
cache_stream = NULL;
On 2010/04/08 18:49:36, stoyan wrote:
> Please add a comment why this should not be CComObjectStackEx.

Done.

http://codereview.chromium.org/1589013/diff/4004/47003#newcode239
chrome_frame/urlmon_bind_status_callback.cc:239: bool data_fully_available =
!!(bscf &
On 2010/04/08 19:02:00, stoyan wrote:
> can you rename this to "force_determination" or similar for clarity? I was
just
> going to write comment that 
> if (data_sniffer_.is_undetermined())
>    return S_OK;
> should fall trough if data_fully_avaialble is true.

Done.

http://codereview.chromium.org/1589013/diff/4004/47004
File chrome_frame/urlmon_bind_status_callback.h (right):

http://codereview.chromium.org/1589013/diff/4004/47004#newcode27
chrome_frame/urlmon_bind_status_callback.h:27: static HRESULT
BSCBFeedData(IBindStatusCallback* bscb, const char* data,
On 2010/04/08 18:49:36, stoyan wrote:
> This is the only public (and used) function of CacheStream class, right? You
can
> hide everything else (and even move this into a impl file.)

currently yes. This class is generic enough that someone else could hold onto
its instance for other uses?

http://codereview.chromium.org/1589013/diff/4004/47005
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/4004/47005#newcode268
chrome_frame/urlmon_moniker.cc:268: if ((S_OK == hr) && callback)
On 2010/04/08 18:49:36, stoyan wrote:
> add a comment, please

Done.

[stoyan, 2010-04-08 23:28:55]

http://codereview.chromium.org/1589013/diff/74002/44032
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/74002/44032#newcode29
chrome_frame/urlmon_bind_status_callback.cc:29: // onto the steam pointer.
steam -> stream

http://codereview.chromium.org/1589013/diff/74002/44034
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/74002/44034#newcode138
chrome_frame/urlmon_moniker.cc:138: // Mark a bind context for navigation by
storing a bind context param.
remove spaces before the comment.

http://codereview.chromium.org/1589013/diff/74002/44040
File chrome_frame/urlmon_moniker.h (right):

http://codereview.chromium.org/1589013/diff/74002/44040#newcode46
chrome_frame/urlmon_moniker.h:46: //    - iff the url is a top level url, we
switch in our own callback object
iff -> if

[ananta, 2010-04-08 23:29:37]

Getting closer. Comments mostly nits below.

http://codereview.chromium.org/1589013/diff/74002/44048
File chrome_frame/test/urlmon_moniker_integration_test.cc (right):

http://codereview.chromium.org/1589013/diff/74002/44048#newcode264
chrome_frame/test/urlmon_moniker_integration_test.cc:264: /*
Can these tests be removed if they are no longer applicable?

http://codereview.chromium.org/1589013/diff/74002/44049
File chrome_frame/test/urlmon_moniker_tests.h (right):

http://codereview.chromium.org/1589013/diff/74002/44049#newcode60
chrome_frame/test/urlmon_moniker_tests.h:60: BEGIN_COM_MAP(MockBindCtxImpl)
Fix alignment here and below

http://codereview.chromium.org/1589013/diff/74002/44049#newcode101
chrome_frame/test/urlmon_moniker_tests.h:101: BEGIN_COM_MAP(MockMonikerImpl)
Ditto for alignment.

http://codereview.chromium.org/1589013/diff/74002/44032
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/1589013/diff/74002/44032#newcode24
chrome_frame/urlmon_bind_status_callback.cc:24: NOTREACHED();
We should probably add a trace here for debugging.

http://codereview.chromium.org/1589013/diff/74002/44032#newcode60
chrome_frame/urlmon_bind_status_callback.cc:60: // read with 0 bytes signalling
caller of the end of data.
indicating that no more data is available?

http://codereview.chromium.org/1589013/diff/74002/44032#newcode69
chrome_frame/urlmon_bind_status_callback.cc:69: CopyMemory(pv, cache_ +
position_, *read);
memcpy instead?

http://codereview.chromium.org/1589013/diff/74002/44032#newcode104
chrome_frame/urlmon_bind_status_callback.cc:104: const size_t kChunkSize = 4 *
1024;
Perhaps this const can be defined outside the loop.

http://codereview.chromium.org/1589013/diff/74002/44032#newcode292
chrome_frame/urlmon_bind_status_callback.cc:292:
CallbackImpl::OnProgress(i->progress_, i->progress_max_,
The OnProgress implementation should be able to handle an empty string?. If yes
then please remove this code.

http://codereview.chromium.org/1589013/diff/74002/44033
File chrome_frame/urlmon_bind_status_callback.h (right):

http://codereview.chromium.org/1589013/diff/74002/44033#newcode93
chrome_frame/urlmon_bind_status_callback.h:93: typedef BSCBImpl CallbackImpl;
Perhaps this should be named as BaseCallbackImpl?

http://codereview.chromium.org/1589013/diff/74002/44033#newcode102
chrome_frame/urlmon_bind_status_callback.h:102: HRESULT MayPlayBack();
Could you add some comments for MayPlayBack?

http://codereview.chromium.org/1589013/diff/74002/44033#newcode107
chrome_frame/urlmon_bind_status_callback.h:107: STDMETHOD(OnDataAvailable)(DWORD
bscf, DWORD size, FORMATETC* format_etc,
better name than bscf?

http://codereview.chromium.org/1589013/diff/74002/44033#newcode114
chrome_frame/urlmon_bind_status_callback.h:114: struct Progress {
Some comments here about the usage of the Progress structure would be great. We
don't add an _ to the member names in the structure.

http://codereview.chromium.org/1589013/diff/74002/44034
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1589013/diff/74002/44034#newcode152
chrome_frame/urlmon_moniker.cc:152: if (FAILED(hr) && cf_doc)
You don't need the cf_doc check here?

http://codereview.chromium.org/1589013/diff/74002/44034#newcode239
chrome_frame/urlmon_moniker.cc:239: if (persist_moniker) {
We should add traces if we fail to QI IPersistMoniker or if the Load call fails
below, etc.

http://codereview.chromium.org/1589013/diff/74002/44034#newcode272
chrome_frame/urlmon_moniker.cc:272: if ((S_OK == hr) && callback)
hr == S_OK

[stoyan, 2010-04-08 23:32:34]

lgtm (with the offline-not-yet-uploaded-fixes)

[amit, 2010-04-08 23:49:01]

Comments addressed, please contine.

On Thu, Apr 8, 2010 at 4:32 PM, <stoyan@chromium.org> wrote:

> lgtm (with the offline-not-yet-uploaded-fixes)
>
>
> http://codereview.chromium.org/1589013/show
>

[ananta, 2010-04-09 00:12:13]

LGTM

[tfarina (gmail-do not use), 2010-04-09 00:35:51]

http://codereview.chromium.org/1589013/diff/6003/70014
File chrome_frame/bho.h (right):

http://codereview.chromium.org/1589013/diff/6003/70014#newcode1
chrome_frame/bho.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
nit: 2010, here and in other files too.

[amit, 2010-04-09 01:12:39]

On 2010/04/09 00:12:13, ananta wrote:
> LGTM

Thanks for the comments so far. I have checked in this patch (disabled by
default) since I want to have QA look at it tomorrow. Comments are still welcome
and I will address them in subsequent CLs.

[tommi (sloooow) - chröme, 2010-04-09 01:44:55]

sounds good.  lgtm!

On Thu, Apr 8, 2010 at 9:12 PM, <amit@chromium.org> wrote:

> On 2010/04/09 00:12:13, ananta wrote:
>
>> LGTM
>>
>
> Thanks for the comments so far. I have checked in this patch (disabled by
> default) since I want to have QA look at it tomorrow. Comments are still
> welcome
> and I will address them in subsequent CLs.
>
>
>
> http://codereview.chromium.org/1589013/show
>