Avoid blocking the PNaCl translator processes when chrome NaCl GDB flag is on.

Add flag to avoid blocking the PNaCl translator when chrome NaCl GDB flag is on.

Augment the NaCl GDB mask to avoid blocking the PNaCl
translator in addition to secure shell. Make this the
default.

This has the side-effect of identifying the translator as
separate from the application in chrome's task manager,
since the "url" is changed for these helper NaCl modules
from the application manifest to something
like chrome://pnacl-translator/x86-64/pnacl-llc.nexe.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3765

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255543

[teravest, 2014-02-25 18:51:28]

https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:172:
pattern.SetValidSchemes(URLPattern::SCHEME_ALL);
I'm confused. Why do you want to set the scheme here, even when the pattern
isn't parsed successfully?

It seems like the existing code already sets valid schemes properly for
 all parsed patterns. Is the problem that Parse() fails without
SetValidSchemes()?


It's weird to have "pattern.SetValidSchemes(URLPattern::SCHEME_ALL)" called
twice here. I don't understand why that would be necessary.

https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:118: // Supply
a fake URL for the translator components so that NaCl GDB
How is the url fake?

[jvoung (off chromium), 2014-02-25 23:52:31]

https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:172:
pattern.SetValidSchemes(URLPattern::SCHEME_ALL);
On 2014/02/25 18:51:29, teravest wrote:
> I'm confused. Why do you want to set the scheme here, even when the pattern
> isn't parsed successfully?
> 
> It seems like the existing code already sets valid schemes properly for
>  all parsed patterns. Is the problem that Parse() fails without
> SetValidSchemes()?

Yes, the problem is that Parse() would otherwise fail when it sees a pattern
like "chrome://*/foo*". It would however be happy with the less strict
"*://*/foo*".


> 
> 
> It's weird to have "pattern.SetValidSchemes(URLPattern::SCHEME_ALL)" called
> twice here. I don't understand why that would be necessary.



There's two points at which the scheme matter:

(1) When you are setting up the pattern matcher. E.g., when you're trying to
parse *://ssh*, or chrome://*

(2) When you're doing the match against a real URL. E.g.,
chrome-extension://abcdefg/ssh_client.nmf.

The added early setting for SCHEME_ALL helps with case (1).

The existing setting for SCHEME_ALL helps with *:// in case (1) plus
chrome-extensions://abcdefb/ssh_client.nmf in case (2). It did not require
setting SCHEME_ALL prior to Parse() because the pattern had a wild card scheme.

I believe that if we had made the SSH pattern more strict like
"chrome-extension://*/*ssh_client.nmf instead of having a wild card at the
schema position, then the pattern matcher would not have reset to only allowing
http and https.

So, we could change the pattern and get rid of the second SetValidSchemes(). An
argument to keep the second one is if there is a chance that someone specifies a
filter pattern manually on the commandline and puts a *:// even though they knew
it only needed to match chrome-extension://.

As for why set it unconditionally to begin with, even when Parse() returns
false? I don't think that matters much since the pattern won't get added to the
debug_patterns_ vector and will be dropped.

https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:118: // Supply
a fake URL for the translator components so that NaCl GDB
On 2014/02/25 18:51:29, teravest wrote:
> How is the url fake?

Perhaps taking the word "fake" out will be less confusing.

What I was thinking is that you can usually type a URL into the omnibar (even if
it was chrome://), and there would be handler. For these components, the only
handler is the pnacl_resources code along with "WrapperForUrl". Otherwise, it's
just a string for displaying on the task manager and for filtering in this CL.

[teravest, 2014-02-26 17:09:31]

On Tue, Feb 25, 2014 at 4:52 PM,  <jvoung@chromium.org> wrote:
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
> File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
> chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:172:
> pattern.SetValidSchemes(URLPattern::SCHEME_ALL);
> On 2014/02/25 18:51:29, teravest wrote:
>>
>> I'm confused. Why do you want to set the scheme here, even when the
>
> pattern
>>
>> isn't parsed successfully?
>
>
>> It seems like the existing code already sets valid schemes properly
>
> for
>>
>>   all parsed patterns. Is the problem that Parse() fails without
>> SetValidSchemes()?
>
>
> Yes, the problem is that Parse() would otherwise fail when it sees a
> pattern like "chrome://*/foo*". It would however be happy with the less
> strict "*://*/foo*".
>
>
>
>
>
>> It's weird to have "pattern.SetValidSchemes(URLPattern::SCHEME_ALL)"
>
> called
>>
>> twice here. I don't understand why that would be necessary.
>
>
>
>
> There's two points at which the scheme matter:
>
> (1) When you are setting up the pattern matcher. E.g., when you're
> trying to parse *://ssh*, or chrome://*
>
> (2) When you're doing the match against a real URL. E.g.,
> chrome-extension://abcdefg/ssh_client.nmf.
>
> The added early setting for SCHEME_ALL helps with case (1).
>
> The existing setting for SCHEME_ALL helps with *:// in case (1) plus
> chrome-extensions://abcdefb/ssh_client.nmf in case (2). It did not
> require setting SCHEME_ALL prior to Parse() because the pattern had a
> wild card scheme.
>
> I believe that if we had made the SSH pattern more strict like
> "chrome-extension://*/*ssh_client.nmf instead of having a wild card at
> the schema position, then the pattern matcher would not have reset to
> only allowing http and https.
>
> So, we could change the pattern and get rid of the second
> SetValidSchemes(). An argument to keep the second one is if there is a
> chance that someone specifies a filter pattern manually on the
> commandline and puts a *:// even though they knew it only needed to
> match chrome-extension://.

Hmm. So does pattern.Parse() change the value of the pattern's valid schemes?
My point is that SetValidSchemes() is called twice with the same
argument, which looks very strange.

>
> As for why set it unconditionally to begin with, even when Parse()
> returns false? I don't think that matters much since the pattern won't
> get added to the debug_patterns_ vector and will be dropped.
>
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
> (right):
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:118: //
> Supply a fake URL for the translator components so that NaCl GDB
> On 2014/02/25 18:51:29, teravest wrote:
>>
>> How is the url fake?
>
>
> Perhaps taking the word "fake" out will be less confusing.
>
> What I was thinking is that you can usually type a URL into the omnibar
> (even if it was chrome://), and there would be handler. For these
> components, the only handler is the pnacl_resources code along with
> "WrapperForUrl". Otherwise, it's just a string for displaying on the
> task manager and for filtering in this CL.

Yeah, it would be less confusing if you didn't call it fake here.

>
> https://codereview.chromium.org/176813010/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/groups/opt_out.

[teravest, 2014-02-26 17:09:33]

On Tue, Feb 25, 2014 at 4:52 PM,  <jvoung@chromium.org> wrote:
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
> File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
> chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:172:
> pattern.SetValidSchemes(URLPattern::SCHEME_ALL);
> On 2014/02/25 18:51:29, teravest wrote:
>>
>> I'm confused. Why do you want to set the scheme here, even when the
>
> pattern
>>
>> isn't parsed successfully?
>
>
>> It seems like the existing code already sets valid schemes properly
>
> for
>>
>>   all parsed patterns. Is the problem that Parse() fails without
>> SetValidSchemes()?
>
>
> Yes, the problem is that Parse() would otherwise fail when it sees a
> pattern like "chrome://*/foo*". It would however be happy with the less
> strict "*://*/foo*".
>
>
>
>
>
>> It's weird to have "pattern.SetValidSchemes(URLPattern::SCHEME_ALL)"
>
> called
>>
>> twice here. I don't understand why that would be necessary.
>
>
>
>
> There's two points at which the scheme matter:
>
> (1) When you are setting up the pattern matcher. E.g., when you're
> trying to parse *://ssh*, or chrome://*
>
> (2) When you're doing the match against a real URL. E.g.,
> chrome-extension://abcdefg/ssh_client.nmf.
>
> The added early setting for SCHEME_ALL helps with case (1).
>
> The existing setting for SCHEME_ALL helps with *:// in case (1) plus
> chrome-extensions://abcdefb/ssh_client.nmf in case (2). It did not
> require setting SCHEME_ALL prior to Parse() because the pattern had a
> wild card scheme.
>
> I believe that if we had made the SSH pattern more strict like
> "chrome-extension://*/*ssh_client.nmf instead of having a wild card at
> the schema position, then the pattern matcher would not have reset to
> only allowing http and https.
>
> So, we could change the pattern and get rid of the second
> SetValidSchemes(). An argument to keep the second one is if there is a
> chance that someone specifies a filter pattern manually on the
> commandline and puts a *:// even though they knew it only needed to
> match chrome-extension://.

Hmm. So does pattern.Parse() change the value of the pattern's valid schemes?
My point is that SetValidSchemes() is called twice with the same
argument, which looks very strange.

>
> As for why set it unconditionally to begin with, even when Parse()
> returns false? I don't think that matters much since the pattern won't
> get added to the debug_patterns_ vector and will be dropped.
>
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
> (right):
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:118: //
> Supply a fake URL for the translator components so that NaCl GDB
> On 2014/02/25 18:51:29, teravest wrote:
>>
>> How is the url fake?
>
>
> Perhaps taking the word "fake" out will be less confusing.
>
> What I was thinking is that you can usually type a URL into the omnibar
> (even if it was chrome://), and there would be handler. For these
> components, the only handler is the pnacl_resources code along with
> "WrapperForUrl". Otherwise, it's just a string for displaying on the
> task manager and for filtering in this CL.

Yeah, it would be less confusing if you didn't call it fake here.

>
> https://codereview.chromium.org/176813010/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jvoung (off chromium), 2014-02-26 18:34:15]

On 2014/02/26 17:09:33, teravest wrote:
> On Tue, Feb 25, 2014 at 4:52 PM,  <mailto:jvoung@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
> > File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):
> >
> >
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
> > chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:172:
> > pattern.SetValidSchemes(URLPattern::SCHEME_ALL);
> > On 2014/02/25 18:51:29, teravest wrote:
> >>
> >> I'm confused. Why do you want to set the scheme here, even when the
> >
> > pattern
> >>
> >> isn't parsed successfully?
> >
> >
> >> It seems like the existing code already sets valid schemes properly
> >
> > for
> >>
> >>   all parsed patterns. Is the problem that Parse() fails without
> >> SetValidSchemes()?
> >
> >
> > Yes, the problem is that Parse() would otherwise fail when it sees a
> > pattern like "chrome://*/foo*". It would however be happy with the less
> > strict "*://*/foo*".
> >
> >
> >
> >
> >
> >> It's weird to have "pattern.SetValidSchemes(URLPattern::SCHEME_ALL)"
> >
> > called
> >>
> >> twice here. I don't understand why that would be necessary.
> >
> >
> >
> >
> > There's two points at which the scheme matter:
> >
> > (1) When you are setting up the pattern matcher. E.g., when you're
> > trying to parse *://ssh*, or chrome://*
> >
> > (2) When you're doing the match against a real URL. E.g.,
> > chrome-extension://abcdefg/ssh_client.nmf.
> >
> > The added early setting for SCHEME_ALL helps with case (1).
> >
> > The existing setting for SCHEME_ALL helps with *:// in case (1) plus
> > chrome-extensions://abcdefb/ssh_client.nmf in case (2). It did not
> > require setting SCHEME_ALL prior to Parse() because the pattern had a
> > wild card scheme.
> >
> > I believe that if we had made the SSH pattern more strict like
> > "chrome-extension://*/*ssh_client.nmf instead of having a wild card at
> > the schema position, then the pattern matcher would not have reset to
> > only allowing http and https.
> >
> > So, we could change the pattern and get rid of the second
> > SetValidSchemes(). An argument to keep the second one is if there is a
> > chance that someone specifies a filter pattern manually on the
> > commandline and puts a *:// even though they knew it only needed to
> > match chrome-extension://.
> 
> Hmm. So does pattern.Parse() change the value of the pattern's valid schemes?
> My point is that SetValidSchemes() is called twice with the same
> argument, which looks very strange.

Yes pattern.Parse() mutates the valid_schemes_ field when it sees a "*://". That
is tickled by our use of the gdb patterns:

!*://*/*ssh_client.nmf
*://*/*debug.nmf

The first we can change to chrome-extension://*/*ssh_client.nmf to not tickle
that, and the second pattern would need to explicitly list out every scheme that
we imagine is legal for .nmf files.

I've changed it to only one "SetValidSchemes()" call, by merging the first
SetValidSchemes() w/ the construction instead.



> 
> >
> > As for why set it unconditionally to begin with, even when Parse()
> > returns false? I don't think that matters much since the pattern won't
> > get added to the debug_patterns_ vector and will be dropped.
> >
> >
> >
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
> > File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
> > ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:118: //
> > Supply a fake URL for the translator components so that NaCl GDB
> > On 2014/02/25 18:51:29, teravest wrote:
> >>
> >> How is the url fake?
> >
> >
> > Perhaps taking the word "fake" out will be less confusing.
> >
> > What I was thinking is that you can usually type a URL into the omnibar
> > (even if it was chrome://), and there would be handler. For these
> > components, the only handler is the pnacl_resources code along with
> > "WrapperForUrl". Otherwise, it's just a string for displaying on the
> > task manager and for filtering in this CL.
> 
> Yeah, it would be less confusing if you didn't call it fake here.
> 

Okay replaced.

[teravest, 2014-02-26 18:36:51]

lgtm

Thanks for explaining this, the interface to URLPattern sure seems strange.

On Wed, Feb 26, 2014 at 11:34 AM,  <jvoung@chromium.org> wrote:
> On 2014/02/26 17:09:33, teravest wrote:
>
>> On Tue, Feb 25, 2014 at 4:52 PM,  <mailto:jvoung@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
>>
>> > File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
>>
>> > chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:172:
>> > pattern.SetValidSchemes(URLPattern::SCHEME_ALL);
>> > On 2014/02/25 18:51:29, teravest wrote:
>> >>
>> >> I'm confused. Why do you want to set the scheme here, even when the
>> >
>> > pattern
>> >>
>> >> isn't parsed successfully?
>> >
>> >
>> >> It seems like the existing code already sets valid schemes properly
>> >
>> > for
>> >>
>> >>   all parsed patterns. Is the problem that Parse() fails without
>> >> SetValidSchemes()?
>> >
>> >
>> > Yes, the problem is that Parse() would otherwise fail when it sees a
>> > pattern like "chrome://*/foo*". It would however be happy with the less
>> > strict "*://*/foo*".
>> >
>> >
>> >
>> >
>> >
>> >> It's weird to have "pattern.SetValidSchemes(URLPattern::SCHEME_ALL)"
>> >
>> > called
>> >>
>> >> twice here. I don't understand why that would be necessary.
>> >
>> >
>> >
>> >
>> > There's two points at which the scheme matter:
>> >
>> > (1) When you are setting up the pattern matcher. E.g., when you're
>> > trying to parse *://ssh*, or chrome://*
>> >
>> > (2) When you're doing the match against a real URL. E.g.,
>> > chrome-extension://abcdefg/ssh_client.nmf.
>> >
>> > The added early setting for SCHEME_ALL helps with case (1).
>> >
>> > The existing setting for SCHEME_ALL helps with *:// in case (1) plus
>> > chrome-extensions://abcdefb/ssh_client.nmf in case (2). It did not
>> > require setting SCHEME_ALL prior to Parse() because the pattern had a
>> > wild card scheme.
>> >
>> > I believe that if we had made the SSH pattern more strict like
>> > "chrome-extension://*/*ssh_client.nmf instead of having a wild card at
>> > the schema position, then the pattern matcher would not have reset to
>> > only allowing http and https.
>> >
>> > So, we could change the pattern and get rid of the second
>> > SetValidSchemes(). An argument to keep the second one is if there is a
>> > chance that someone specifies a filter pattern manually on the
>> > commandline and puts a *:// even though they knew it only needed to
>> > match chrome-extension://.
>
>
>> Hmm. So does pattern.Parse() change the value of the pattern's valid
>> schemes?
>> My point is that SetValidSchemes() is called twice with the same
>> argument, which looks very strange.
>
>
> Yes pattern.Parse() mutates the valid_schemes_ field when it sees a "*://".
> That
> is tickled by our use of the gdb patterns:
>
> !*://*/*ssh_client.nmf
> *://*/*debug.nmf
>
> The first we can change to chrome-extension://*/*ssh_client.nmf to not
> tickle
> that, and the second pattern would need to explicitly list out every scheme
> that
> we imagine is legal for .nmf files.
>
> I've changed it to only one "SetValidSchemes()" call, by merging the first
> SetValidSchemes() w/ the construction instead.
>
>
>
>
>
>> >
>> > As for why set it unconditionally to begin with, even when Parse()
>> > returns false? I don't think that matters much since the pattern won't
>> > get added to the debug_patterns_ vector and will be dropped.
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
>>
>> > File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
>> > (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
>>
>> > ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:118: //
>> > Supply a fake URL for the translator components so that NaCl GDB
>> > On 2014/02/25 18:51:29, teravest wrote:
>> >>
>> >> How is the url fake?
>> >
>> >
>> > Perhaps taking the word "fake" out will be less confusing.
>> >
>> > What I was thinking is that you can usually type a URL into the omnibar
>> > (even if it was chrome://), and there would be handler. For these
>> > components, the only handler is the pnacl_resources code along with
>> > "WrapperForUrl". Otherwise, it's just a string for displaying on the
>> > task manager and for filtering in this CL.
>
>
>> Yeah, it would be less confusing if you didn't call it fake here.
>
>
>
> Okay replaced.
>
> https://codereview.chromium.org/176813010/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/groups/opt_out.

[teravest, 2014-02-26 18:36:51]

lgtm

Thanks for explaining this, the interface to URLPattern sure seems strange.

On Wed, Feb 26, 2014 at 11:34 AM,  <jvoung@chromium.org> wrote:
> On 2014/02/26 17:09:33, teravest wrote:
>
>> On Tue, Feb 25, 2014 at 4:52 PM,  <mailto:jvoung@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
>>
>> > File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/chrome/browser/nacl_host...
>>
>> > chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:172:
>> > pattern.SetValidSchemes(URLPattern::SCHEME_ALL);
>> > On 2014/02/25 18:51:29, teravest wrote:
>> >>
>> >> I'm confused. Why do you want to set the scheme here, even when the
>> >
>> > pattern
>> >>
>> >> isn't parsed successfully?
>> >
>> >
>> >> It seems like the existing code already sets valid schemes properly
>> >
>> > for
>> >>
>> >>   all parsed patterns. Is the problem that Parse() fails without
>> >> SetValidSchemes()?
>> >
>> >
>> > Yes, the problem is that Parse() would otherwise fail when it sees a
>> > pattern like "chrome://*/foo*". It would however be happy with the less
>> > strict "*://*/foo*".
>> >
>> >
>> >
>> >
>> >
>> >> It's weird to have "pattern.SetValidSchemes(URLPattern::SCHEME_ALL)"
>> >
>> > called
>> >>
>> >> twice here. I don't understand why that would be necessary.
>> >
>> >
>> >
>> >
>> > There's two points at which the scheme matter:
>> >
>> > (1) When you are setting up the pattern matcher. E.g., when you're
>> > trying to parse *://ssh*, or chrome://*
>> >
>> > (2) When you're doing the match against a real URL. E.g.,
>> > chrome-extension://abcdefg/ssh_client.nmf.
>> >
>> > The added early setting for SCHEME_ALL helps with case (1).
>> >
>> > The existing setting for SCHEME_ALL helps with *:// in case (1) plus
>> > chrome-extensions://abcdefb/ssh_client.nmf in case (2). It did not
>> > require setting SCHEME_ALL prior to Parse() because the pattern had a
>> > wild card scheme.
>> >
>> > I believe that if we had made the SSH pattern more strict like
>> > "chrome-extension://*/*ssh_client.nmf instead of having a wild card at
>> > the schema position, then the pattern matcher would not have reset to
>> > only allowing http and https.
>> >
>> > So, we could change the pattern and get rid of the second
>> > SetValidSchemes(). An argument to keep the second one is if there is a
>> > chance that someone specifies a filter pattern manually on the
>> > commandline and puts a *:// even though they knew it only needed to
>> > match chrome-extension://.
>
>
>> Hmm. So does pattern.Parse() change the value of the pattern's valid
>> schemes?
>> My point is that SetValidSchemes() is called twice with the same
>> argument, which looks very strange.
>
>
> Yes pattern.Parse() mutates the valid_schemes_ field when it sees a "*://".
> That
> is tickled by our use of the gdb patterns:
>
> !*://*/*ssh_client.nmf
> *://*/*debug.nmf
>
> The first we can change to chrome-extension://*/*ssh_client.nmf to not
> tickle
> that, and the second pattern would need to explicitly list out every scheme
> that
> we imagine is legal for .nmf files.
>
> I've changed it to only one "SetValidSchemes()" call, by merging the first
> SetValidSchemes() w/ the construction instead.
>
>
>
>
>
>> >
>> > As for why set it unconditionally to begin with, even when Parse()
>> > returns false? I don't think that matters much since the pattern won't
>> > get added to the debug_patterns_ vector and will be dropped.
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
>>
>> > File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
>> > (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/176813010/diff/40001/ppapi/native_client/src/...
>>
>> > ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:118: //
>> > Supply a fake URL for the translator components so that NaCl GDB
>> > On 2014/02/25 18:51:29, teravest wrote:
>> >>
>> >> How is the url fake?
>> >
>> >
>> > Perhaps taking the word "fake" out will be less confusing.
>> >
>> > What I was thinking is that you can usually type a URL into the omnibar
>> > (even if it was chrome://), and there would be handler. For these
>> > components, the only handler is the pnacl_resources code along with
>> > "WrapperForUrl". Otherwise, it's just a string for displaying on the
>> > task manager and for filtering in this CL.
>
>
>> Yeah, it would be less confusing if you didn't call it fake here.
>
>
>
> Okay replaced.
>
> https://codereview.chromium.org/176813010/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jvoung (off chromium), 2014-03-06 18:45:56]

+sdk folks
Brad and Sam, I've changed the default to what we discussed.

[bradn, 2014-03-06 18:56:34]

lgtm

https://codereview.chromium.org/176813010/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/176813010/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:194: {
IDS_NACL_DEBUG_MASK_CHOICE_EXCLUDE_UTILS_PNACL, "", "" },
Maybe pull out the stuff in parens to:
NOTE: As the default value must be the empty string, the mask excluding the
PNaCl translator and secure shell is substituted in elsewhere.

https://codereview.chromium.org/176813010/diff/100001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/176813010/diff/100001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:350: nacl_debug_mask =
"!*://*/*ssh_client.nmf,chrome://pnacl-translator/*";
{
}
goto fail :-) ?

https://codereview.chromium.org/176813010/diff/100001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/176813010/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:438: NaClSubprocess*
Plugin::LoadHelperNaClModule(const nacl::string &helper_url,
string&

[jvoung (off chromium), 2014-03-06 19:02:51]

Thanks!

https://codereview.chromium.org/176813010/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/176813010/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:194: {
IDS_NACL_DEBUG_MASK_CHOICE_EXCLUDE_UTILS_PNACL, "", "" },
On 2014/03/06 18:56:34, bradn wrote:
> Maybe pull out the stuff in parens to:
> NOTE: As the default value must be the empty string, the mask excluding the
> PNaCl translator and secure shell is substituted in elsewhere.

Done.

https://codereview.chromium.org/176813010/diff/100001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/176813010/diff/100001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:350: nacl_debug_mask =
"!*://*/*ssh_client.nmf,chrome://pnacl-translator/*";
On 2014/03/06 18:56:34, bradn wrote:
> {
> }
> goto fail :-) ?

Done.

https://codereview.chromium.org/176813010/diff/100001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/176813010/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:438: NaClSubprocess*
Plugin::LoadHelperNaClModule(const nacl::string &helper_url,
On 2014/03/06 18:56:34, bradn wrote:
> string&

Done.

[jvoung (off chromium), 2014-03-06 19:05:25]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2014-03-06 19:09:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/176813010/120001

[commit-bot: I haz the power, 2014-03-06 20:59:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/176813010/120001

[commit-bot: I haz the power, 2014-03-06 21:52:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/176813010/120001

[commit-bot: I haz the power, 2014-03-07 08:33:12]

Change committed as 255543