Use incognito split for ChromeVox and TTS in Guest mode

Use incognito split for ChromeVox and TTS in Guest mode

+ enable HTML5 filesystem in TTS in Guest mode

BUG=337639
TEST=manual

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

[Dmitry Polukhin, 2014-02-05 20:14:18]

PTAL

[Dmitry Polukhin, 2014-02-05 23:04:29]

+ tzik@ for OWNER review in webkit/browser/fileapi/

[asargent_no_longer_on_chrome, 2014-02-06 00:58:02]

https://codereview.chromium.org/141733008/diff/250001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/141733008/diff/250001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:201: #endif
It seems a little hacky to inject a value into the parsed manifest like this - I
can see someone in the future not knowing about this corner of code getting
confused and/or breaking something because they were looking at the manifest in
the resources file and assuming that's what we're actually loading. 

Didn't we already fix this in a different way for the file manager?

[Dmitry Polukhin, 2014-02-06 01:30:21]

https://codereview.chromium.org/141733008/diff/250001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/141733008/diff/250001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:201: #endif
On 2014/02/06 00:58:02, Antony Sargent wrote:
> It seems a little hacky to inject a value into the parsed manifest like this -
I
> can see someone in the future not knowing about this corner of code getting
> confused and/or breaking something because they were looking at the manifest
in
> the resources file and assuming that's what we're actually loading. 
> 
> Didn't we already fix this in a different way for the file manager?

I'm not aware of the fix for the file manager. Do you know where is it?

For most other apps like QuickOffice and Feedback we just added incognito:split
into the manifest file. But I can't do it for ChromeVox and TTS because it
breaks normal case when incognito profile is loaded (two background pages for
such system wide things can't coexist). I would love to fix it another way but
so far it the simplest approach that works. We will need to merge in into M33 to
fix regression. Alternative approach is to create two manifest files one for
normal case and another one for Guest mode. I choose this approach to avoid
problem with duplicating manifest files.

Also there is bug about Japanese keyboard that is extension
(https://code.google.com/p/chromium/issues/detail?id=339318), I suspect it is
the same root cause.

[tzik, 2014-02-06 04:09:54]

fileapi changes LGTM.

[Dmitry Polukhin, 2014-02-06 23:43:17]

Antony, please take another look. As we discussed offline I created separate
manifest files for Gust mode case.

[asargent_no_longer_on_chrome, 2014-02-07 00:09:50]

lgtm

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:606: // TODO(dpolukhin): Hack to
enable HTML5 temporary file system for
Please file a bug to fix this (is this an extensions infrastructure thing?) and
link to it here, so we don't forget to clean this up.

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.h:144: void
EnbaleFileSystemInGuestMode(const std::string& id);
nit: typo in method name "Enbale" -> "Enable"

[Dmitry Polukhin, 2014-02-07 00:19:59]

+ jhawkins@ for OWNER review in chrome/browser

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:606: // TODO(dpolukhin): Hack to
enable HTML5 temporary file system for
On 2014/02/07 00:09:50, Antony Sargent wrote:
> Please file a bug to fix this (is this an extensions infrastructure thing?)
and
> link to it here, so we don't forget to clean this up.

It is not extensions issue. Root of the problem that HTML FileSystem doesn't
work in Incognito mode. We have this bug for a long time
https://code.google.com/p/chromium/issues/detail?id=93417 Temporary FileSystem
should just work in incognito mode.

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/141733008/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.h:144: void
EnbaleFileSystemInGuestMode(const std::string& id);
On 2014/02/07 00:09:50, Antony Sargent wrote:
> nit: typo in method name "Enbale" -> "Enable"

Done.

[James Hawkins, 2014-02-07 16:45:09]

lgtm

[Dmitry Polukhin, 2014-02-07 16:47:40]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-02-07 16:48:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/141733008/430001

[commit-bot: I haz the power, 2014-02-07 16:48:47]

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

[commit-bot: I haz the power, 2014-02-07 16:48:47]

Failed to apply patch for chrome/browser/extensions/component_loader.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/component_loader.cc
  Hunk #1 FAILED at 335.
  Hunk #2 succeeded at 479 (offset -11 lines).
  Hunk #3 succeeded at 513 (offset -11 lines).
  Hunk #4 succeeded at 580 (offset -15 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/extensions/component_loader.cc.rej

Patch:       chrome/browser/extensions/component_loader.cc
Index: chrome/browser/extensions/component_loader.cc
diff --git a/chrome/browser/extensions/component_loader.cc
b/chrome/browser/extensions/component_loader.cc
index
0ef7a9f06ac1ebf687cedc88c247795aa21abe19..459e461cf7bc1324db32ae08995f8c535972f452
100644
--- a/chrome/browser/extensions/component_loader.cc
+++ b/chrome/browser/extensions/component_loader.cc
@@ -335,8 +335,12 @@ void ComponentLoader::AddNetworkSpeechSynthesisExtension()
{
 
 #if defined(OS_CHROMEOS)
 void ComponentLoader::AddChromeOsSpeechSynthesisExtension() {
-  Add(IDR_SPEECH_SYNTHESIS_MANIFEST,
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  int idr = command_line->HasSwitch(chromeos::switches::kGuestSession) ?
+      IDR_SPEECH_SYNTHESIS_GUEST_MANIFEST : IDR_SPEECH_SYNTHESIS_MANIFEST;
+  std::string id = Add(idr,
       base::FilePath(extension_misc::kSpeechSynthesisExtensionPath));
+  EnableFileSystemInGuestMode(id);
 }
 #endif
 
@@ -490,21 +494,7 @@ void
ComponentLoader::AddDefaultComponentExtensionsWithBackgroundPages(
             chromeos::switches::kDisableQuickofficeComponentApp)) {
       std::string id = Add(IDR_QUICKOFFICE_MANIFEST, base::FilePath(
           FILE_PATH_LITERAL("/usr/share/chromeos-assets/quick_office")));
-      if (command_line->HasSwitch(chromeos::switches::kGuestSession)) {
-        // TODO(dpolukhin): Hack to enable HTML5 temporary file system for
-        // Quickoffice. It doesn't work without temporary file system access.
-        // Make sure temporary file system is enabled in the off the record
-        // browser context (as that is the one used in guest session).
-        content::BrowserContext* off_the_record_context =
-            ExtensionsBrowserClient::Get()->GetOffTheRecordContext(
-                browser_context_);
-        GURL site = content::SiteInstance::GetSiteForURL(
-            off_the_record_context, Extension::GetBaseURLFromExtensionId(id));
-        fileapi::FileSystemContext* file_system_context =
-            content::BrowserContext::GetStoragePartitionForSite(
-                off_the_record_context, site)->GetFileSystemContext();
-        file_system_context->EnableTemporaryFileSystemInIncognito();
-      }
+      EnableFileSystemInGuestMode(id);
     }
 #endif  // defined(GOOGLE_CHROME_BUILD)
 
@@ -538,9 +528,9 @@ void
ComponentLoader::AddDefaultComponentExtensionsWithBackgroundPages(
   // Load ChromeVox extension now if spoken feedback is enabled.
   if (chromeos::AccessibilityManager::Get() &&
       chromeos::AccessibilityManager::Get()->IsSpokenFeedbackEnabled()) {
-    base::FilePath path =
-        base::FilePath(extension_misc::kChromeVoxExtensionPath);
-    Add(IDR_CHROMEVOX_MANIFEST, path);
+    int idr = command_line->HasSwitch(chromeos::switches::kGuestSession) ?
+        IDR_CHROMEVOX_GUEST_MANIFEST : IDR_CHROMEVOX_MANIFEST;
+    Add(idr, base::FilePath(extension_misc::kChromeVoxExtensionPath));
   }
 #endif  // defined(OS_CHROMEOS)
 
@@ -609,4 +599,25 @@ void
ComponentLoader::UnloadComponent(ComponentExtensionInfo* component) {
   }
 }
 
+void ComponentLoader::EnableFileSystemInGuestMode(const std::string& id) {
+#if defined(OS_CHROMEOS)
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(chromeos::switches::kGuestSession)) {
+    // TODO(dpolukhin): Hack to enable HTML5 temporary file system for
+    // the extension. Some component extensions don't work without temporary
+    // file system access. Make sure temporary file system is enabled in the
off
+    // the record browser context (as that is the one used in guest session).
+    content::BrowserContext* off_the_record_context =
+        ExtensionsBrowserClient::Get()->GetOffTheRecordContext(
+            browser_context_);
+    GURL site = content::SiteInstance::GetSiteForURL(
+        off_the_record_context, Extension::GetBaseURLFromExtensionId(id));
+    fileapi::FileSystemContext* file_system_context =
+        content::BrowserContext::GetStoragePartitionForSite(
+            off_the_record_context, site)->GetFileSystemContext();
+    file_system_context->EnableTemporaryFileSystemInIncognito();
+  }
+#endif
+}
+
 }  // namespace extensions

[Dmitry Polukhin, 2014-02-07 18:02:36]

+ Peter for accessibility review

Please take another look after non-trivial rebase due to conflicts with
https://codereview.chromium.org/149233004

[Peter Lundblad, 2014-02-07 18:54:43]

Hi,

Thanks, great that you only use split mode in guest mode so we get the old
behaviour in regular sessions!  In the interest of avoiding code duplication,
would it be too crazy to just add the incognito value to the dictionary
after loading the manifest when in guest mode for these two extensios?
(I know myself too well to think I wouldn't forget to modify the
guest version when modifying the other one in the future;)

//Peter


-- 

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

[asargent_no_longer_on_chrome, 2014-02-07 19:16:54]

That was the initial approach, but I was concerned that it would be very
confusing if at some point in the future someone working on this was
looking at the manifest and seeing behavior that wasn't specified in the
manifest, because in this corner of the code they didn't know about, we
were modifying the contents on the fly. Having 2 copies is terrible also,
for the reason you point out. I filed crbug.com/341680 for us on the
extensions team to add better infastructure. Perhaps that could be a more
flexible declaration of the incognito mode? (eg allow a separate value for
guest sessions?)


On Fri, Feb 7, 2014 at 10:54 AM, <plundblad@chromium.org> wrote:

> Hi,
>
> Thanks, great that you only use split mode in guest mode so we get the old
> behaviour in regular sessions!  In the interest of avoiding code
> duplication,
> would it be too crazy to just add the incognito value to the dictionary
> after loading the manifest when in guest mode for these two extensios?
> (I know myself too well to think I wouldn't forget to modify the
> guest version when modifying the other one in the future;)
>
> //Peter
>
>
> --
>

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

[Dmitry Polukhin, 2014-02-07 20:25:26]

Peter, do you still have unresolved concerns about manifest files duplication?
Proposed solution is not elegant but reliable and highly visible.

[Peter Lundblad, 2014-02-07 21:22:15]

dpolukhin@chromium.org writes:
> Peter, do you still have unresolved concerns about manifest files  
> duplication?
> Proposed solution is not elegant but reliable and highly visible.
> 
Sorry to insist.  Seems like we support // comments in the manifest.json.
Could we just have a NOTE that guest mode enables split mode instead.
It will be easy to miss the extra file in code reviews, but if you debug
issues related to guest mode, you probably start by looking at the manifest.
I hope we won't have to go futher down this path, however.

Thanks,
//Peter

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

[Dmitry Polukhin, 2014-02-07 21:34:59]

On 2014/02/07 21:22:15, Peter Lundblad wrote:
> mailto:dpolukhin@chromium.org writes:
> > Peter, do you still have unresolved concerns about manifest files  
> > duplication?
> > Proposed solution is not elegant but reliable and highly visible.
> > 
> Sorry to insist.  Seems like we support // comments in the manifest.json.
> Could we just have a NOTE that guest mode enables split mode instead.
> It will be easy to miss the extra file in code reviews, but if you debug
> issues related to guest mode, you probably start by looking at the manifest.
> I hope we won't have to go futher down this path, however.

I have no personal preferences and can return code from patchset #1. But
argument with comment inside manifest.json can be turned in opposite direction
and I can add comment in both manifest files that they should be in sync.
Antony, what do you think?

[Peter Lundblad, 2014-02-07 22:06:35]

lgtm

I would prefer to not have this code duplication for the reason I mentioned in
my previous comment, but I am not blocking this fix on that.
In case you didn't notice, would simplify things if you wait on
https://codereview.chromium.org/149233004
to be merged before merging this to 1750 because the revert readds a file
that you touch.

//Peter

dpolukhin@chromium.org writes:
> On 2014/02/07 21:22:15, Peter Lundblad wrote:
> > mailto:dpolukhin@chromium.org writes:
> > > Peter, do you still have unresolved concerns about manifest files
> > > duplication?
> > > Proposed solution is not elegant but reliable and highly visible.
> > >
> > Sorry to insist.  Seems like we support // comments in the manifest.json.
> > Could we just have a NOTE that guest mode enables split mode instead.
> > It will be easy to miss the extra file in code reviews, but if you debug
> > issues related to guest mode, you probably start by looking at the  
> > manifest.
> > I hope we won't have to go futher down this path, however.
> 
> I have no personal preferences and can return code from patchset #1. But
> argument with comment inside manifest.json can be turned in opposite  
> direction
> and I can add comment in both manifest files that they should be in sync.
> Antony, what do you think?
> 
> https://codereview.chromium.org/141733008/

-- 

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

[Dmitry Polukhin, 2014-02-07 22:31:30]

On 2014/02/07 22:06:35, Peter Lundblad wrote:
> I would prefer to not have this code duplication for the reason I mentioned in
> my previous comment, but I am not blocking this fix on that.
> In case you didn't notice, would simplify things if you wait on
> https://codereview.chromium.org/149233004
> to be merged before merging this to 1750 because the revert readds a file
> that you touch.

I added comment about keeping manifest files in sync. As for cl/149233004,
unfortunately it didn't simplify this CL and I already rebase on top of it.

[Dmitry Polukhin, 2014-02-07 22:32:28]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-02-07 22:35:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/141733008/720001

[Peter Lundblad, 2014-02-07 22:50:37]

dpolukhin@chromium.org writes:
> On 2014/02/07 22:06:35, Peter Lundblad wrote:
> > I would prefer to not have this code duplication for the reason I  
> > mentioned in
> > my previous comment, but I am not blocking this fix on that.
> > In case you didn't notice, would simplify things if you wait on
> > https://codereview.chromium.org/149233004
> > to be merged before merging this to 1750 because the revert readds a file
> > that you touch.
> 
> I added comment about keeping manifest files in sync. As for cl/149233004,
> unfortunately it didn't simplify this CL and I already rebase on top of it.

I was talking about merging back to the 1750 branch (M33).
Since you're CL touches a file that doesn't exist on that branch, but will
come back after 149233004 is merged
(chrome/browser/speech/tts_extension_loader_chromeos.cc).  I was just suggesting
that we merge 149233004 before this cl.  Hope this clarifies.

//Peter

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

-- 

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

[commit-bot: I haz the power, 2014-02-08 01:54:58]

Change committed as 249888