Fix #1515 related issues in PR #1508 for new tabs

[wadkar]

PR #1508 introduced another issue that setting vimium's newTabURL to
the inbuilt pages/blank.html resulted in opening a new tab with search
query set to pages/blank.html.

This commit solves the issue by conditionally calling Utils.convertToUrl.
Also, it changes openOptionsPageInNewTab call to use
openUrlInNewTab for consistency and reduce usage of
chrome.tabs.create API call.

Note that this may not solve #1507 as chrome seems to open
chrome-extension://... URLs in non-incognito window by design.
You may want to set it to (some other blank page)[1] so that it can be
accessed from incognito mode. You can always open chrome://newtab from
incognito window.

[1] http://this-page-intentionally-left-blank.org/

[wadkar]

This commit is based off PR #1508. Do you want me to rebase it to some other branch/current master? If you could resolve the conflicts yourself, that will be great though.

[smblott-github]

An alternative would be cced8f7, which just fills in the absolute URL for pages/blank.html directly. cced8f7 differs from what's here if the new-tab URL is set to options.html (cced8f7 sends it to the default search engine, whereas this PR opens the options page). It's not at all clear which is correct. However, pages/blank.html is the only URL which we've suggested should work as a new-tab option.

(It looks like there may be no way to use pages/blank.html in incognito mode.)

[wadkar]

Hmm, alternative cced8f7 works too. In that case, we might include @mrmr1993's suggestion for openOptionsPageInNewTab. See my comment on cced8f7.
Feel free to close this PR in case cced8f7 is accepted.
On a side note, how do I keep in sync with upstream for build, test, as well as running bleeding edge vimium? It feels silly when I cant see cced8f7 on my local.

[smblott-github]

@wadkar. Thanks for this. But I think we should go with cced8f7.

[mrmr1993]

@smblott-github Can we still explore having a unified tab opening function? It seems like calling the same chrome API in slightly different ways with different levels of detail throughout the codebase is generally not the best practise.

Some other thoughts:

• openUrlInNewTab should probably only open an url in a new tab (so no conversion to a search url; the frontend should point to a different function called eg. openUrlOrSearchInNewTab which does this and then calls openUrlInNewTab itself)
• hardcoding pages/blank.html as an exception is bad practise and a cheap fix (sorry).

[smblott-github]

It seems like calling the same chrome API in slightly different ways with different levels of detail throughout the codebase is generally not the best practise.

If we create our own centralised function, we'll just end up calling it in slightly different ways with different levels of detail throughout the codebase. Or, put another way, we'd just be creating our own chrome.tabs.create API. It might be worth doing that, at some point. But probably only if we need a consistent API which is clearly different from chrome.tabs.create.

hardcoding pages/blank.html as an exception is bad practise and a cheap fix (sorry).

Fair, but also a bit harsh. createTab is th only place where pages/blank.html has a spacial meaning.

Changing topic (and mentioning you, @mrmr1993, to get your attention)...

I'm wondering whether we shouldn't abandon our new-tab page functionality entirely. We can't make t and Ctrl-T do the same thing, and we can't get pages/blank.html to work in incognito-mode windows. That's no great. Personally, I just use my little new-tab extension (which solves the first problem, but not the second). If Vimium provided a reliable blank page somewhere with a very-long TTL, we could point users towards that.