Add capability to simple http server to bound Set-Cookie to URLs.

Add capability to simple http server to bound Set-Cookie to URL.

Update simple http server so that user specified cookies can be included in the headers of http response.

BUG=

Committed: https://crrev.com/203bbc711d1134ad2b5d0dc88bf656d3ceb83341
Cr-Commit-Position: refs/heads/master@{#416304}

[liaoyuke, 2016-08-30 23:47:49]

Description was changed from

==========
Add capability to simple http server to bound Set-Cookie to URL.

BUG=
==========

to

==========
Add capability to simple http server to bound Set-Cookie to URL.

Update simple http server so that user specified cookies can be included in the
headers of http response.

BUG=
==========

[liaoyuke, 2016-08-30 23:47:50]

liaoyuke@chromium.org changed reviewers:
+ baxley@chromium.org, pkl@chromium.org

[liaoyuke, 2016-08-30 23:48:00]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 23:48:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 00:41:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 00:41:16]

Dry run: This issue passed the CQ dry run.

[pkl (ping after 24h if needed), 2016-09-01 00:18:15]

Would it be possible to have unit tests added to the various response_providers?

https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
File ios/web/public/test/response_providers/html_response_provider_impl.mm
(right):

https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
ios/web/public/test/response_providers/html_response_provider_impl.mm:28: for
(const std::pair<const GURL, std::pair<std::string, std::string>>& pair :
Can auto type make this for loop more readable?

for (const auto& pair : responses) {
  ...
}

[pkl (ping after 24h if needed), 2016-09-01 00:31:07]

On 2016/09/01 00:18:15, pklpkl wrote:
> Would it be possible to have unit tests added to the various
response_providers?
> 
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> File ios/web/public/test/response_providers/html_response_provider_impl.mm
> (right):
> 
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> ios/web/public/test/response_providers/html_response_provider_impl.mm:28: for
> (const std::pair<const GURL, std::pair<std::string, std::string>>& pair :
> Can auto type make this for loop more readable?
> 
> for (const auto& pair : responses) {
>   ...
> }

BTW, I know there wasn't any unit tests to begin with. I'm OK for the short
term.
It would be good to track the addition of unit tests on the test framework :)

[liaoyuke, 2016-09-01 00:40:16]

Yeah, I had a strong desire adding some unit tests for my CL, but didn't do
that because there are too few examples, and it's not that obvious for me
to figure out how to use the C++ test framework.

But since you brought this up, I think I'm gonna throw all my excuses away
and do that, I really would like to see more unit tests being added to the
code base.

Thank you very much!

On Wed, Aug 31, 2016 at 5:18 PM <pkl@chromium.org> wrote:

> Would it be possible to have unit tests added to the various
> response_providers?
>
>
>
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> File
> ios/web/public/test/response_providers/html_response_provider_impl.mm
> (right):
>
>
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> ios/web/public/test/response_providers/html_response_provider_impl.mm:28:
>
> for (const std::pair<const GURL, std::pair<std::string, std::string>>&
> pair :
> Can auto type make this for loop more readable?
>
> for (const auto& pair : responses) {
> ...
> }
>
> https://codereview.chromium.org/2291393003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkl (ping after 24h if needed), 2016-09-01 00:41:53]

It feels acceptable to submit this and add unit tests as a separate task. baxley
can chime in.

On 2016/09/01 00:40:16, liaoyuke wrote:
> Yeah, I had a strong desire adding some unit tests for my CL, but didn't do
> that because there are too few examples, and it's not that obvious for me
> to figure out how to use the C++ test framework.
> 
> But since you brought this up, I think I'm gonna throw all my excuses away
> and do that, I really would like to see more unit tests being added to the
> code base.
> 
> Thank you very much!
> 
> On Wed, Aug 31, 2016 at 5:18 PM <mailto:pkl@chromium.org> wrote:
> 
> > Would it be possible to have unit tests added to the various
> > response_providers?
> >
> >
> >
> >
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> > File
> > ios/web/public/test/response_providers/html_response_provider_impl.mm
> > (right):
> >
> >
> >
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> > ios/web/public/test/response_providers/html_response_provider_impl.mm:28:
> >
> > for (const std::pair<const GURL, std::pair<std::string, std::string>>&
> > pair :
> > Can auto type make this for loop more readable?
> >
> > for (const auto& pair : responses) {
> > ...
> > }
> >
> > https://codereview.chromium.org/2291393003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[liaoyuke, 2016-09-01 00:43:59]

Good idea! I can also do that.

On Wed, Aug 31, 2016 at 5:41 PM <pkl@chromium.org> wrote:

> It feels acceptable to submit this and add unit tests as a separate task.
> baxley
> can chime in.
>
>
> On 2016/09/01 00:40:16, liaoyuke wrote:
> > Yeah, I had a strong desire adding some unit tests for my CL, but didn't
> do
> > that because there are too few examples, and it's not that obvious for me
> > to figure out how to use the C++ test framework.
> >
> > But since you brought this up, I think I'm gonna throw all my excuses
> away
> > and do that, I really would like to see more unit tests being added to
> the
> > code base.
> >
> > Thank you very much!
> >
> > On Wed, Aug 31, 2016 at 5:18 PM <mailto:pkl@chromium.org> wrote:
> >
> > > Would it be possible to have unit tests added to the various
> > > response_providers?
> > >
> > >
> > >
> > >
> >
>
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> > > File
> > > ios/web/public/test/response_providers/html_response_provider_impl.mm
> > > (right):
> > >
> > >
> > >
> >
>
>
https://codereview.chromium.org/2291393003/diff/20001/ios/web/public/test/res...
> > > ios/web/public/test/response_providers/
> html_response_provider_impl.mm:28:
> > >
> > > for (const std::pair<const GURL, std::pair<std::string, std::string>>&
> > > pair :
> > > Can auto type make this for loop more readable?
> > >
> > > for (const auto& pair : responses) {
> > > ...
> > > }
> > >
> > > https://codereview.chromium.org/2291393003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2291393003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[baxley, 2016-09-01 04:23:09]

I think we should wait for the short term (i.e. C-A-T).

Since nothing in place, some thought needs to be done on how to do them. For
example, will we run them on a bot? Or just expect developers to run them when
they make changes? It's not just writing tests, and it's not just setting up a
test harness for them, but it also means deciding the infrastructure to run
them.

Anyway, now isn't the time to pick, I just want to make sure we're deliberate.
We should also weigh it against creating a new https server which is the next
big task coming. If it has a completely different API, then we'd have to start
over.

To be clear, I'm not saying to not write unit tests. :) I filed crbug.com/643042
to track it. I think the CL is okay, we should put our effort to getting out of
the convert-a-thon first. Once that's done, then we can more seriously pursue
this. What do you think?

[liaoyuke, 2016-09-01 05:46:25]

Yes, that's sounds good! I'll land this CL first and then maybe add unit
tests as a separate task.

Thanks,
Yuke

On Wed, Aug 31, 2016 at 9:23 PM <baxley@chromium.org> wrote:

> I think we should wait for the short term (i.e. C-A-T).
>
> Since nothing in place, some thought needs to be done on how to do them.
> For
> example, will we run them on a bot? Or just expect developers to run them
> when
> they make changes? It's not just writing tests, and it's not just setting
> up a
> test harness for them, but it also means deciding the infrastructure to run
> them.
>
> Anyway, now isn't the time to pick, I just want to make sure we're
> deliberate.
> We should also weigh it against creating a new https server which is the
> next
> big task coming. If it has a completely different API, then we'd have to
> start
> over.
>
> To be clear, I'm not saying to not write unit tests. :) I filed
> crbug.com/643042
> to track it. I think the CL is okay, we should put our effort to getting
> out of
> the convert-a-thon first. Once that's done, then we can more seriously
> pursue
> this. What do you think?
>
> https://codereview.chromium.org/2291393003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[liaoyuke, 2016-09-02 16:30:01]

Addressed previous comments, please take another look.

Thanks

[pkl (ping after 24h if needed), 2016-09-02 16:40:51]

lgtm

[liaoyuke, 2016-09-02 16:54:32]

The CQ bit was checked by liaoyuke@chromium.org

[commit-bot: I haz the power, 2016-09-02 16:54:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 16:54:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 16:54:51]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[baxley, 2016-09-02 17:33:29]

On 2016/09/02 16:54:51, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. 
> CQ run can only be started by full committers or once the patch has
> received an L-G-T-M from a full committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

lgtm

[liaoyuke, 2016-09-02 18:00:12]

The CQ bit was checked by liaoyuke@chromium.org

[commit-bot: I haz the power, 2016-09-02 18:00:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 18:39:36]

Description was changed from

==========
Add capability to simple http server to bound Set-Cookie to URL.

Update simple http server so that user specified cookies can be included in the
headers of http response.

BUG=
==========

to

==========
Add capability to simple http server to bound Set-Cookie to URL.

Update simple http server so that user specified cookies can be included in the
headers of http response.

BUG=
==========

[commit-bot: I haz the power, 2016-09-02 18:39:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 18:41:15]

Description was changed from

==========
Add capability to simple http server to bound Set-Cookie to URL.

Update simple http server so that user specified cookies can be included in the
headers of http response.

BUG=
==========

to

==========
Add capability to simple http server to bound Set-Cookie to URL.

Update simple http server so that user specified cookies can be included in the
headers of http response.

BUG=

Committed: https://crrev.com/203bbc711d1134ad2b5d0dc88bf656d3ceb83341
Cr-Commit-Position: refs/heads/master@{#416304}
==========

[commit-bot: I haz the power, 2016-09-02 18:41:15]

Patchset 3 (id:??) landed as
https://crrev.com/203bbc711d1134ad2b5d0dc88bf656d3ceb83341
Cr-Commit-Position: refs/heads/master@{#416304}

[liaoyuke, 2016-09-13 00:31:38]

Patchset #4 (id:60001) has been deleted