Set the attach file checkbox to checked when launched from an extension.

Set the attach file checkbox to checked when launched from an extension.
If we're launched from an extension with the filepath given, set the checkbox to checked.
This CL also fixes a bug which was preventing attach file from an extension from working.


R=dbeam@chromium.org
BUG=169982
TEST=Attach a file from the QO extension and confirm that everything works.

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

[Dan Beam, 2013-04-16 22:01:12]

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.html (right):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.html:57: <input
id="attach-file-custom-checkbox" type="checkbox" checked>
I don't see where this differentiates whether this is an extension or not?

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:309: if (queryPos !== -1) {
what is the point of this diff?

[rkc, 2013-04-16 22:07:34]

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.html (right):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.html:57: <input
id="attach-file-custom-checkbox" type="checkbox" checked>
On 2013/04/16 22:01:12, Dan Beam wrote:
> I don't see where this differentiates whether this is an extension or not?

This entire section is hidden - it only get's unhidden if we get invoked with
filePath argument, which presumably will only come when we're navigated to by an
extension.

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:309: if (queryPos !== -1) {
On 2013/04/16 22:01:12, Dan Beam wrote:
> what is the point of this diff?

I had changed this to use window.location.search (as per your review comment on
a previous CL) but that broke the case in which the code invoking it provides a
#0 on the href (as older extensions do). This code searches for the first ? to
seperate out the query string from everything else - I can change this to
something else if it works better?

[Dan Beam, 2013-04-16 23:20:18]

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:309: if (queryPos !== -1) {
On 2013/04/16 22:07:34, Rahul Chaturvedi wrote:
> On 2013/04/16 22:01:12, Dan Beam wrote:
> > what is the point of this diff?
> 
> I had changed this to use window.location.search (as per your review comment
on
> a previous CL) but that broke the case in which the code invoking it provides
a
> #0 on the href (as older extensions do). This code searches for the first ? to
> seperate out the query string from everything else - I can change this to
> something else if it works better?

can you give me a full URL of an example that doesn't work?

[rkc, 2013-04-16 23:22:58]

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (right):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:309: if (queryPos !== -1) {
On 2013/04/16 23:20:18, Dan Beam wrote:
> On 2013/04/16 22:07:34, Rahul Chaturvedi wrote:
> > On 2013/04/16 22:01:12, Dan Beam wrote:
> > > what is the point of this diff?
> > 
> > I had changed this to use window.location.search (as per your review comment
> on
> > a previous CL) but that broke the case in which the code invoking it
provides
> a
> > #0 on the href (as older extensions do). This code searches for the first ?
to
> > seperate out the query string from everything else - I can change this to
> > something else if it works better?
> 
> can you give me a full URL of an example that doesn't work?

chrome://feedback/#0?description=&categoryTag=FromQuickoffice&customPageUrl=chrome-extension://gbkeegbaiigmenfmjfclcdgdpimamgkj/views/qowt.html&filePath=/Downloads/resume.doc

[Dan Beam, 2013-04-16 23:28:12]

On 2013/04/16 23:22:58, Rahul Chaturvedi wrote:
>
https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
> File chrome/browser/resources/feedback.js (right):
> 
>
https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
> chrome/browser/resources/feedback.js:309: if (queryPos !== -1) {
> On 2013/04/16 23:20:18, Dan Beam wrote:
> > On 2013/04/16 22:07:34, Rahul Chaturvedi wrote:
> > > On 2013/04/16 22:01:12, Dan Beam wrote:
> > > > what is the point of this diff?
> > > 
> > > I had changed this to use window.location.search (as per your review
comment
> > on
> > > a previous CL) but that broke the case in which the code invoking it
> provides
> > a
> > > #0 on the href (as older extensions do). This code searches for the first
?
> to
> > > seperate out the query string from everything else - I can change this to
> > > something else if it works better?
> > 
> > can you give me a full URL of an example that doesn't work?
> 
>
chrome://feedback/#0?description=&categoryTag=FromQuickoffice&customPageUrl=chrome-extension://gbkeegbaiigmenfmjfclcdgdpimamgkj/views/qowt.html&filePath=/Downloads/resume.doc

what happens in the case of

  chrome://feedback?yada=yada#0?blah=blah

?

[Dan Beam, 2013-04-16 23:34:04]

this would pick up both, also can you write a test for this?

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (left):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:308: var query =
window.location.search.substr(1).split('&');
var loc = window.location;
var query = loc.search.substr(1).split('&');
if (loc.hash.indexOf('?') >= 0)
  query = query.concat(loc.hash.split('?').slice(1).join('').split('&'));

[rkc, 2013-04-17 00:38:07]

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
File chrome/browser/resources/feedback.js (left):

https://codereview.chromium.org/13853008/diff/1/chrome/browser/resources/feed...
chrome/browser/resources/feedback.js:308: var query =
window.location.search.substr(1).split('&');
On 2013/04/16 23:34:05, Dan Beam wrote:
> var loc = window.location;
> var query = loc.search.substr(1).split('&');
> if (loc.hash.indexOf('?') >= 0)
>   query = query.concat(loc.hash.split('?').slice(1).join('').split('&'));

Modified this a bit to make it slightly more understandable (at least to me) and
added comments.
Done.

[rkc, 2013-04-17 00:39:43]

I have an open item to add tests for the feedback page, I'll make sure I add a
test for this code when I get to that work item.

[Dan Beam, 2013-04-17 00:53:13]

lgtm

[Dan Beam, 2013-04-17 00:53:27]

On 2013/04/17 00:39:43, Rahul Chaturvedi wrote:
> I have an open item to add tests for the feedback page, I'll make sure I add a
> test for this code when I get to that work item.

What is the milestone of this work?  M28?

[rkc, 2013-04-17 01:30:39]

On 2013/04/17 00:53:27, Dan Beam wrote:
> On 2013/04/17 00:39:43, Rahul Chaturvedi wrote:
> > I have an open item to add tests for the feedback page, I'll make sure I add
a
> > test for this code when I get to that work item.
> 
> What is the milestone of this work?  M28?

https://code.google.com/p/chromium/issues/detail?id=181707

[rkc, 2013-04-17 21:18:52]

Committed patchset #2 manually as r194678 (presubmit successful).